Thanks - committed. On Tue, Feb 23, 2016 at 1:34 PM, Sanket Mehta <sanket.me...@enterprisedb.com > wrote:
> Hi, > > PFA the revised patch as per your comments. > Please review it and let me know the feedback. > > Regards, > Sanket Mehta > Sr Software engineer > Enterprisedb > > On Tue, Feb 23, 2016 at 4:10 PM, Dave Page <dp...@pgadmin.org> wrote: > >> Hi >> >> I've attached an update to this patch, in which I've done some >> word-smithing on various comments, and adjusted the SQL templates to >> improve the formatting. >> >> However, it looks like it's bit-rotted, as the dependents/dependencies >> display is throwing Python errors. Please fix and then I think it's just >> about ready to commit. >> >> Thanks. >> >> >> On Fri, Feb 19, 2016 at 11:03 AM, Sanket Mehta < >> sanket.me...@enterprisedb.com> wrote: >> >>> Hi Dave, >>> >>> PFA the revise patch. >>> >>> It includes changes according to your review comments as well as >>> dependency/dependent part also. >>> >>> Let me know in case anything is missing. >>> >>> Regards, >>> Sanket Mehta >>> Sr Software engineer >>> Enterprisedb >>> >>> On Mon, Feb 15, 2016 at 10:25 PM, Dave Page <dp...@pgadmin.org> wrote: >>> >>>> 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 >>>> >>> >>> >> >> >> -- >> 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
