Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.80) (envelope-from ) id 1aVMOh-0004S2-VX for pgadmin-hackers@arkaria.postgresql.org; Mon, 15 Feb 2016 16:53:16 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84) (envelope-from ) id 1aVMOh-000642-IC for pgadmin-hackers@arkaria.postgresql.org; Mon, 15 Feb 2016 16:53:15 +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 1aVMOg-00063t-OD for pgadmin-hackers@postgresql.org; Mon, 15 Feb 2016 16:53:15 +0000 Received: from mail-ig0-x22b.google.com ([2607:f8b0:4001:c05::22b]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84) (envelope-from ) id 1aVMOd-0007lt-Lf for pgadmin-hackers@postgresql.org; Mon, 15 Feb 2016 16:53:13 +0000 Received: by mail-ig0-x22b.google.com with SMTP id y8so78876635igp.0 for ; Mon, 15 Feb 2016 08:53:11 -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=VZl6KqBWa3blycBWLNu8rAs5bXfl0RAELODGcqVxnXk=; b=qFrymHM3mc0Or5wE6uiCfGaLQ4fuNfrhTFeDvWOvWnvGnzxDxoupjXPjuWfRgPi+Kh z1phx38prkYIgv0rjoCokh7zQN3MObJOkkPTnKjCW5s0gyv+ekW6zl64QveRUcZSp0f3 vip1ckEBWT8t2xTeeqSEM0PFxlH1X4psJDcfou+Rvy3pTlqjCdgdT7izRWBv58q9ScuV KIasbxCQ2q4oabxk2cPFA3mBaq5WhnIcKhbR+ayUtwStgcQc85TuJvM62vmUru3yOuEy YWzow2spp5/0ZxY0YgzCQlj92nm/9hygzJqtTWDDf0+OSiRGzZddY3fZtUs07eoUkt+l /E9w== 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=VZl6KqBWa3blycBWLNu8rAs5bXfl0RAELODGcqVxnXk=; b=bqFacgV28hy53FahXivS7bI7a9tBl8wQUM/FrdhxYpHSUq6hbm4Km3VVHKOgQqge8f 7ic+hi9yBxNIw9BJW2kQuCfAoXgGpVjm4QdUx3m4TjJo0FaOkX4vzj7YLQSgps9ZlUI8 Ut4nUz9bU+vL8TOFtg981pxKEzyXU8QMnLtEMz9hDjUGBMD5mbDt4WJ4nqVnMt8WfEGg vj/WPacbXEsd1gRFYAAkWI2vcgeOvCTI59rXhq3a8/ZB/+xUfkFJRAd7fOCFF85MfIWj eO3Q+N38DNGI71uZ3ViUxXG3F6dXjiFTtot7HBYs/kEfIr+O6VcfDRsuQUisopSkcDkL 6VmQ== X-Gm-Message-State: AG10YORhaEUlQPoEUhUMw6A+nwiAaz09nVVx0SXYJBh+/YEgeH29Pa4Rw7Q51rAHft/+kifd14CwEBVOS5KBdg== MIME-Version: 1.0 X-Received: by 10.50.57.101 with SMTP id h5mr7440729igq.96.1455555190680; Mon, 15 Feb 2016 08:53:10 -0800 (PST) Received: by 10.64.76.137 with HTTP; Mon, 15 Feb 2016 08:53:10 -0800 (PST) In-Reply-To: References: Date: Mon, 15 Feb 2016 16:53:10 +0000 Message-ID: Subject: Re: patch for cast module From: Dave Page To: Sanket Mehta Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary=047d7bdc14a41791f5052bd1da45 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 --047d7bdc14a41791f5052bd1da45 Content-Type: text/plain; charset=UTF-8 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 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 --047d7bdc14a41791f5052bd1da45 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
That's much better. Just a couple of comments now, par= tly based on an email I wrote earlier:

- There is still = inconsistency in comment style. Please see the attachment for an example. N= ote that there is *always* a space between the comment marker and text.

- If I try to edit a cast, I can change the descripti= on - but no SQL is shown on the SQL tab, despite the comment being correctl= y applied when I hit save. The properties pane of the main window is also n= ot 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 patc= h with all the required comments.



Regards,Sanket Mehta
Sr Software engineer
Enterprisedb
=

On Mon, Feb 15= , 2016 at 4:18 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">


On Mon, Feb 15, 2016 at 8:10 AM, Sa= nket 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 co= ntrols if any extended.

Can you please let me know where exact= ly you think more comments are required?

<= /span>
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 n= odes in isolation, it's extremely difficult to get a sense of what exac= tly each part of the code is doing. In this example, what I see by reading = the code is:

- Define the required libraries (requ= ire.js stuff)
- Extend the collection class
- Extend th= e node class
=C2=A0 - Define an init function inline
= =C2=A0 - Add the menu options

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

- There are no blank lines to separate logical code sections at all b= etween line 48 and 235 (there is one blank line, but it doesn't separat= e code sections).=C2=A0
- There are 4 comments that I can see. Th= e first two are identical, and appear to have identical code blocks followi= ng them for reasons that are not even remotely obvious.=C2=A0
- A= s a newcomer to this code, I'm wondering if it's purpose is to defi= ne 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 info= rmation 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 co= mments, because the point is that without spending a couple of hours unders= tanding this, I simply don't know. The point of the comments (and separ= ation 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, t= hen fix and improve. Be generous with comments, but don't use them unne= cessarily (e.g. "a =3D 1 // Set a to one").

<= div>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.blog= spot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.comThe Enterprise PostgreSQL Company




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

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