Re: patch for cast module

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-19 11:03:50
Message-ID: CA+yw=mN59EY0rB20Y7q9YLJ=vT7tQHv8R0SvZ4QQjkDXwGE1CQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

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
>

Attachment Content-Type Size
castv9.patch text/x-patch 44.8 KB

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Dave Page 2016-02-19 12:15:42 Re: [pgAdmin4][Patch]: Dependecies/Dependents functionality
Previous Message Akshay Joshi 2016-02-19 10:48:17 Re: [pgAdmin4] [Patch]: Language Module