From: | Sanket Mehta <sanket(dot)mehta(at)enterprisedb(dot)com> |
---|---|
To: | Dave Page <dpage(at)pgadmin(dot)org> |
Cc: | pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org> |
Subject: | Re: patch for cast module |
Date: | 2016-02-23 13:34:02 |
Message-ID: | CA+yw=mMnzWiw=NjHfR5VZSF7nZNBdbHU26kDf+Aihm9zobUPUw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgadmin-hackers |
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 <dpage(at)pgadmin(dot)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(dot)mehta(at)enterprisedb(dot)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 <dpage(at)pgadmin(dot)org> wrote:
>>
>>> And this time with the attachment...
>>>
>>> On Mon, Feb 15, 2016 at 4:53 PM, Dave Page <dpage(at)pgadmin(dot)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(dot)mehta(at)enterprisedb(dot)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 <dpage(at)pgadmin(dot)org> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Mon, Feb 15, 2016 at 8:10 AM, Sanket Mehta <
>>>>>> sanket(dot)mehta(at)enterprisedb(dot)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
>
Attachment | Content-Type | Size |
---|---|---|
castv10.patch | text/x-patch | 44.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Sanket Mehta | 2016-02-23 13:44:26 | Re: PATCH: PGADMIN 4 - FTS templates node |
Previous Message | Khushboo Vashi | 2016-02-23 13:23:38 | [pgAdmin4] [Patch]: Foreign Table Module |