Hi Please find updated patch and review.
On Thu, May 18, 2017 at 7:36 PM, Joao Pedro De Almeida Pereira < jdealmeidapere...@pivotal.io> wrote: > Hello Hackers, > > We reviewed the PR, and there are a lot of new lines of code in this > patch, are we sure that we can have a good coverage of the functionality > just by doing feature tests? > Adding more code to a 3.5k+ lines file doesn't look like a good option, do > you think it is possible to extract some of the functionality to their own > files and have test around those functionalities? > To improve the code readability, reduce code complexity and to make code testable, the code must be splitted component wise. Here is my suggestion: 1. The code for classes like “SQLEditorView” and “SqlEditorController” can be moved into two files like "editor_view.js and “editor_controller.js" and called from within "sqleditor.js". 2. All utilities functions can be moved into separate utils file and can write test cases. 3. Slickgrid listener functions such as: onBeforeEditCell, onKeyDown, onCellChange, onAddNewRow can be moved into a file and write test cases around. This needs discussion. Any suggestion? > > Do we really need to have an epicRandomString function in our code? Would > it be better to use a library that would provide us that functionality out > of the box? > We are using "epicRandomString function" to uniquely identify each record, I talked to Harshal who is eliminating the use of this function and instead maintaining counter for the rows added/updated/deleted. > The functions this.applyValue in slick.pgadmin.editors.js that were > change in this patch are exactly the same, can we extract that code into a > single function instead of repeating the code? > I have moved common logic into a new function. > > Using feature tests is a good option to ensure that the integration of all > the components of the application is working as expected, but in order to > tests behaviors that are edge cases the Unit Tests are much cheaper to run > and create. > I will write test cases for functions once they are moved into their separate files as discussed above. > > Thanks > Joao & Shruti > > > On Thu, May 18, 2017 at 1:22 AM, Surinder Kumar < > surinder.ku...@enterprisedb.com> wrote: > >> Hi Dave, >> >> Please review the updated patch. >> >> On Wed, May 17, 2017 at 8:46 PM, Dave Page <dp...@pgadmin.org> wrote: >> >>> Hi >>> >>> On Wed, May 17, 2017 at 3:08 PM, Surinder Kumar < >>> surinder.ku...@enterprisedb.com> wrote: >>> >>>> Hi Dave, >>>> >>>> *Implementation:* >>>> >>>> 1) Took a flag 'is_row_copied' for each copied row: >>>> >>>> - to distinguish it from add/update row. >>>> - to write code specific to copied row only as it requires different >>>> logic than add row. >>>> >>>> 2) After pasting an existing row: >>>> >>>> - If a user clear the cell value, it must set cell to [default] value >>>> if default value is explicitly given for column while creating table >>>> otherwise [null]. >>>> >>>> - Again, if a user entered a value in same cell, then removes that >>>> value, set it to [null] (the same behaviour is for add row). >>>> >>>> 3) Took a 2-dimensional array "grid.copied_rows" to keep track the >>>> changes of each row's cell so that changes made to each cell are >>>> independent and removed once data is saved. >>>> >>>> 4) On pasting a row, the cell must be highlighted with light green >>>> colour, so triggers an addNewRow event. Now copied row will add to grid >>>> instead of re-rendering all rows again. >>>> >>>> 5) Moved the common logic into functions. >>>> >>>> This patch pass the feature test cases and jasmine test case. >>>> Also I will add the test cases for copy row and send an updated patch. >>>> >>>> Please find attached patch and review. >>>> >>> >>> Looks good. I saw the following issues: >>> >>> - The "new" row is not available once I've created the first new row, or >>> pasted some. >>> >> Fixed. >> >>> >>> - Rows are pasted in reverse order. >>> >> Fixed. >> >>> >>> - If I copy/paste a new row that has yet to be saved, none of the values >>> are actually copied. >>> >> Fixed. >> >>> >>> - Attempting to save a row that contains all null/default values results >>> in an invalid query: >>> >>> INSERT INTO public.defaults ( >>> ) VALUES ( >>> ); >>> >>> I think the only answer here is to explicitly insert NULL into any >>> columns *without* a default value. >>> >> I could not reproduce, However #3 might have fixed it too. >> >> Apart from this, I noticed epicRandomString(...) function doesn't >> generate unique number always, due to this save and delete rows was >> affected. Not all rows saved or deleted. The new function always returns a >> unique random number. >> Fixed. >> >>> >>> Thanks. >>> >>> -- >>> Dave Page >>> Blog: http://pgsnake.blogspot.com >>> Twitter: @pgsnake >>> >>> EnterpriseDB UK: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >>> >> >> >> >> -- >> Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) >> To make changes to your subscription: >> http://www.postgresql.org/mailpref/pgadmin-hackers >> >> >
RM_2400_v2.patch
Description: Binary data
-- Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers
