Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.80) (envelope-from ) id 1aYcb6-00031b-Mu for pgadmin-hackers@arkaria.postgresql.org; Wed, 24 Feb 2016 16:47:32 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84) (envelope-from ) id 1aYcb6-0001qz-9p for pgadmin-hackers@arkaria.postgresql.org; Wed, 24 Feb 2016 16:47:32 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84) (envelope-from ) id 1aYcb5-0001oW-6Q for pgadmin-hackers@postgresql.org; Wed, 24 Feb 2016 16:47:31 +0000 Received: from mail-io0-x22b.google.com ([2607:f8b0:4001:c06::22b]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84) (envelope-from ) id 1aYcb2-0006jl-1i for pgadmin-hackers@postgresql.org; Wed, 24 Feb 2016 16:47:30 +0000 Received: by mail-io0-x22b.google.com with SMTP id z135so51408143iof.0 for ; Wed, 24 Feb 2016 08:47:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin-org.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=bu8pV7LAjpazTl08mnITuSrcFW6IkFiRzEgnZRie2Lc=; b=ahhBEy0w4wFH4J3LOtZGcm05BxYUlOfpve+pcYrVqDP+ztG4ePAwRvVSoJr4l7Bwer guRkjQ4iWMdnpbUDC7oYyas6PHQiyaeFoq1qwsZc9WiTJyVYBK8m7tpOORILp73jvvx8 tn12eq7J4y++VgukYdfi45PV5WGLWDvppIJB8e/2hz6cb6z5kq7SOrydHBQngSW7udwB mrpXWPgPc698ilVGx7mmvtcpjhhLQlkptbP3eSAjnFBisFR4soTPWVNk38hg2btMvAEQ v5dQxjc/QmmJE46sKb1HOClM8nDk2KJG3F7x4TqoiHvB888fb3bPGQDrjLZp0dAWv8ik wCWA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=bu8pV7LAjpazTl08mnITuSrcFW6IkFiRzEgnZRie2Lc=; b=JXtdpsojuyXi0EVqBkz9B5ECgZJOi78KAV9fCWAW6s4+f9o6LaoocHpwoZEUA1GdbB PN5WbtOOFEH3YaVG81OWHqULB8ybsWN3c147DpZg2dLj1Se+y2yFbAWhTkzABvBW2kQP ZYFUAt7O03k6Z3seJPq+5RQezHsoApykAyCPc/az/JzGE1m7cot2h1eRZFNjVql6XMUn IjhPkG9db3SyaN4nNkk2wg9hb8HdAqMlnwgG4xhhIDImq6oUduqlBYZMO07s6OQjNgN6 zKwmCZ8ZbnTVdgcYc1DHqWY67amxWzP5XhpUg27OpjPjN/lVtga+VzirA6NMfwgUipwh ep7w== X-Gm-Message-State: AG10YORN5i5x/0DHVsQ9rmYPDlBMnWYy/6mS7kfPpvXOGBaERwGyDGnxkjgO7Vpde51bxbT6G0jSKZ3tSlmx1Q== MIME-Version: 1.0 X-Received: by 10.107.12.226 with SMTP id 95mr41609910iom.70.1456332447313; Wed, 24 Feb 2016 08:47:27 -0800 (PST) Received: by 10.64.90.3 with HTTP; Wed, 24 Feb 2016 08:47:27 -0800 (PST) In-Reply-To: References: Date: Wed, 24 Feb 2016 16:47:27 +0000 Message-ID: Subject: Re: patch for cast module From: Dave Page To: Sanket Mehta Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary=001a113f8f8e329097052c86d21d X-Pg-Spam-Score: -2.6 (--) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgadmin-hackers Precedence: bulk Sender: pgadmin-hackers-owner@postgresql.org --001a113f8f8e329097052c86d21d Content-Type: text/plain; charset=UTF-8 Thanks - committed. On Tue, Feb 23, 2016 at 1:34 PM, Sanket Mehta wrote: > 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 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.mehta@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 wrote: >>> >>>> And this time with the attachment... >>>> >>>> On Mon, Feb 15, 2016 at 4:53 PM, Dave Page 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.mehta@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 wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> On Mon, Feb 15, 2016 at 8:10 AM, Sanket Mehta < >>>>>>> sanket.mehta@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 >> > > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --001a113f8f8e329097052c86d21d Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Thanks - committed.

On Tue, Feb 23, 2016 at 1:34 PM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:
Hi,

PFA the revis= ed patch as per your comments.
Please review it and let me know the feed= back.

Regards,
Sanket Mehta
Sr Softwar= e engineer
Enterprisedb

On Tue, Feb 23= , 2016 at 4:10 PM, Dave Page <dpage@pgadmin.org> wrote:
<= blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px= #ccc solid;padding-left:1ex">
Hi

I'= ve attached an update to this patch, in which I've done some word-smith= ing on various comments, and adjusted the SQL templates to improve the form= atting.

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 <sa= nket.mehta@enterprisedb.com> wrote:
Hi Dave,

PFA the revi= se patch.

It includes changes according to your review comment= s as well as dependency/dependent part also.

Let me know in ca= se anything is missing.

Regards,
Sanket Mehta
Sr = Software engineer
Enterprisedb

On Mon, Feb 15, 2016 at 10:= 25 PM, Dave Page <dpage@pgadmin.org> wrote:
And this time with the attachment...

On Mo= n, Feb 15, 2016 at 4:53 PM, Dave Page <dpage@pgadmin.org> wr= ote:
That's much bet= ter. Just a couple of comments now, partly based on an email I wrote earlie= r:

- There is still inconsistency in comment style. Plea= se see the attachment for an example. Note that there is *always* a space b= etween 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 prope= rties pane of the main window is also not updated.

Otherwise, it looks fine.
<= div class=3D"gmail_extra">
Thanks.

On Mon= , Feb 15, 2016 at 1:28 PM, Sanket Mehta <sanket.mehta@enterpri= sedb.com> wrote:
Hi,

PFA the revised patch with all the required = comments.



Regards,
Sanket Mehta
Sr Softwa= re engineer
Enterprisedb

On Mon, Feb 15, 2016 at 4:1= 8 PM, Dave Page <dpage@pgadmin.org> wrote:


On Mon, Feb 15, 2016 at 8:10 AM, Sanket Mehta <sanket.mehta@enterprisedb.com> wrote:
Hi Dave,

Reg= arding your suggestion of putting some comments in javascript, I think I ha= ve 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 th= e 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 isola= tion, 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:<= /div>

- Define the required libraries (require.js stuff)=
- Extend the collection class
- Extend the node class<= /div>
=C2=A0 - Define an init function inline
=C2=A0 - Add th= e menu options

That part is fairly easy to figure = out (easier because there are blank lines between the logical sections). Fr= om there though, it becomes much harder;

- There a= re 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= ).=C2=A0
- There are 4 comments that I can see. The first two are= identical, and appear to have identical code blocks following them for rea= sons that are not even remotely obvious.=C2=A0
- 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 n= eed 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 logica= l sections of code with blank lines) is to make it easy for another develop= er (especially one as rusty as me) to read and understand, then fix and imp= rove. Be generous with comments, but don't use them unnecessarily (e.g.= "a =3D 1 // Set a to one").

Of course, = this is not just directed at you Sanket - it's something all of us work= ing on pgAdmin need to keep in mind.

Thanks.
=

--
Dave Page
Blog: http://pgsnake.blogspot.comTwitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise= PostgreSQL Company




--
Dave Pa= ge
Blog: http:= //pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterpris= edb.com
The Enterprise PostgreSQL Company



--
Dave Pa= ge
Blog: http:= //pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterpris= edb.com
The Enterprise PostgreSQL Company




--
Dave Pa= ge
Blog: http:= //pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterpris= edb.com
The Enterprise PostgreSQL Company




--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake=

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Compan= y
--001a113f8f8e329097052c86d21d--