And this time with the attachment... On Mon, Feb 15, 2016 at 4:53 PM, Dave Page <dp...@pgadmin.org> wrote:
> That's much better. Just a couple of comments now, partly based on an > email I wrote earlier: > > - There is still inconsistency in comment style. Please see the attachment > for an example. Note that there is *always* a space between the comment > marker and text. > > - If I try to edit a cast, I can change the description - but no SQL is > shown on the SQL tab, despite the comment being correctly applied when I > hit save. The properties pane of the main window is also not updated. > > Otherwise, it looks fine. > > Thanks. > > On Mon, Feb 15, 2016 at 1:28 PM, Sanket Mehta < > sanket.me...@enterprisedb.com> wrote: > >> Hi, >> >> PFA the revised patch with all the required comments. >> >> >> >> Regards, >> Sanket Mehta >> Sr Software engineer >> Enterprisedb >> >> On Mon, Feb 15, 2016 at 4:18 PM, Dave Page <dp...@pgadmin.org> wrote: >> >>> >>> >>> On Mon, Feb 15, 2016 at 8:10 AM, Sanket Mehta < >>> sanket.me...@enterprisedb.com> wrote: >>> >>>> Hi Dave, >>>> >>>> Regarding your suggestion of putting some comments in javascript, I >>>> think I have already put some comments regarding model data and their >>>> controls if any extended. >>>> >>>> Can you please let me know where exactly you think more comments are >>>> required? >>>> >>> >>> Hi >>> >>> The issue for me is that jQuery code isn't the easiest to read at the >>> best of times, with nested/anonymous functions and inline JSON etc. As I >>> look through the code for the various nodes in isolation, it's extremely >>> difficult to get a sense of what exactly each part of the code is doing. In >>> this example, what I see by reading the code is: >>> >>> - Define the required libraries (require.js stuff) >>> - Extend the collection class >>> - Extend the node class >>> - Define an init function inline >>> - Add the menu options >>> >>> That part is fairly easy to figure out (easier because there are blank >>> lines between the logical sections). From there though, it becomes much >>> harder; >>> >>> - There are no blank lines to separate logical code sections at all >>> between line 48 and 235 (there is one blank line, but it doesn't separate >>> code sections). >>> - There are 4 comments that I can see. The first two are identical, and >>> appear to have identical code blocks following them for reasons that are >>> not even remotely obvious. >>> - As a newcomer to this code, I'm wondering if it's purpose is to define >>> the backform model. If so, why is it not broken up into sections with a >>> comment to tell me what field each block handles, and any other useful >>> information I may need to know? If it's not, then what is it for? >>> >>> So... I'm not going to tell you exactly where to put comments, because >>> the point is that without spending a couple of hours understanding this, I >>> simply don't know. The point of the comments (and separation of logical >>> sections of code with blank lines) is to make it easy for another developer >>> (especially one as rusty as me) to read and understand, then fix and >>> improve. Be generous with comments, but don't use them unnecessarily (e.g. >>> "a = 1 // Set a to one"). >>> >>> Of course, this is not just directed at you Sanket - it's something all >>> of us working on pgAdmin need to keep in mind. >>> >>> Thanks. >>> >>> -- >>> Dave Page >>> Blog: http://pgsnake.blogspot.com >>> Twitter: @pgsnake >>> >>> EnterpriseDB UK: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >>> >> >> > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- 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
