Re: patch for cast module

From: Dave Page <dpage(at)pgadmin(dot)org>
To: Sanket Mehta <sanket(dot)mehta(at)enterprisedb(dot)com>
Cc: pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
Subject: Re: patch for cast module
Date: 2016-02-15 16:55:47
Message-ID: CA+OCxoyBC3d1aRO9UsE5wMC8xEmjEMvKBVZ0GmJ8UZtJ9Zyaog@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

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

Attachment Content-Type Size
image/png 17.8 KB

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Sergey Busel 2016-02-16 00:42:52 Re: pgAdmin III: Muliple SQL tabs
Previous Message Dave Page 2016-02-15 16:53:10 Re: patch for cast module