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
castv9-dave.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
