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-23 10:40:46
Message-ID: CA+OCxoyrnS4NKLk-DUvqC45vt8fmDDQAuU6m+AiBX1RoaNroGw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

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
castv9-dave.patch application/octet-stream 43.7 KB

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Surinder Kumar 2016-02-23 11:07:22 Re: [pgAdmin4] [Patch]: Extension Module
Previous Message Dave Page 2016-02-23 10:09:13 Re: [pgAdmin4] [Patch]: Language Module