MIME-Version: 1.0
In-Reply-To: 
 <CA+yw=mMJHCYGVZoSL2sAe9C-x7HSWexsK7SGEapNX=NaWYUPUA@mail.gmail.com>
References: 
 <CA+yw=mPCZvZ7+L+g6peQjEe9cihHFUgT4hW3X9ASTRg3wdJgpQ@mail.gmail.com>
	<CA+yw=mORG-LGpa948F8RzbiQkZ3eUmr_3mgqS+akRGx-640Z+Q@mail.gmail.com>
	<CACCA4P1y5u3kmWn+cV7qb9JKCHrAKw-x4kWmbiHSdrcxDfT0bw@mail.gmail.com>
	<CA+yw=mOi=xbgHophMuhw+D9sdmv=rLoubqG8RALQ+1Hw0nZ92g@mail.gmail.com>
	<CA+yw=mOD9i_tLu4r5qpGw1Gd9PKY3xRg4gDHKm010eKPHiCMmQ@mail.gmail.com>
	<CANxoLDedHH+i9hpma8Q3SUiFWU+fMUO_eGtLU+ECmF4fOii_og@mail.gmail.com>
	<CA+yw=mMPQzqmfcpdoJmrvBoA+wc_0758TNDjrF1w1RXHxYsGDw@mail.gmail.com>
	<CANxoLDdESMArY0HvraikVDtBwE4sFqtn0FpSNkohU1R3_vQsNA@mail.gmail.com>
	<CA+yw=mPjs2xqN162encZzWsdUNYBQvKUzHvPcTg3DLirDSe9fA@mail.gmail.com>
	<CA+OCxow0Q5mLbTijw-fQ2_RGpkFTJ-K9hKFPFFxY-NSTLxgJJw@mail.gmail.com>
	<CA+yw=mM9knpdKBfiP63t1nO=h2PCgWjPaaWnw+eQ1S6AYy7K8A@mail.gmail.com>
	<CA+OCxoxtSr-POJ6LBF8r9cxKXzTCJdWj1iQsp803-y0arrMsHg@mail.gmail.com>
	<CA+yw=mPfUbTk_J-ddArjRCruNGZ0_LFAB6CQY77PODPif8jHsA@mail.gmail.com>
	<CA+OCxoxXjtNab106EK-D=Gzz4boxU=T3J4CaiiMRY+RBPBt+iw@mail.gmail.com>
	<CA+OCxoyBC3d1aRO9UsE5wMC8xEmjEMvKBVZ0GmJ8UZtJ9Zyaog@mail.gmail.com>
	<CA+yw=mN59EY0rB20Y7q9YLJ=vT7tQHv8R0SvZ4QQjkDXwGE1CQ@mail.gmail.com>
	<CA+OCxoyrnS4NKLk-DUvqC45vt8fmDDQAuU6m+AiBX1RoaNroGw@mail.gmail.com>
	<CA+yw=mMnzWiw=NjHfR5VZSF7nZNBdbHU26kDf+Aihm9zobUPUw@mail.gmail.com>
	<CA+OCxoxC5Jn9bGVzqGm6EvhW=ZJ0WaFbTaPAx5a18UuPJXiRBg@mail.gmail.com>
	<CA+yw=mO9QjYBPHBNeJSr3TC-iLxKL1AuwFuvMWCCj-XnQTSsGQ@mail.gmail.com>
	<CA+OCxoyAMmG88zNTGSLjdLpomwc9JHksULQQD80xrr3b8WZLqA@mail.gmail.com>
	<CA+yw=mOnUgMo2ASPUqH-g6ZQUPXQtDHs-yn-3tJQnkCJp=HNMw@mail.gmail.com>
	<CA+yw=mMJHCYGVZoSL2sAe9C-x7HSWexsK7SGEapNX=NaWYUPUA@mail.gmail.com>
Date: Thu, 17 Mar 2016 11:49:50 +0000
Message-ID: 
 <CA+OCxoxWad42aBeEUTDGbMSPO_GMbkYc=weGdt6JyDoy-ipc4w@mail.gmail.com>
Subject: Re: patch for cast module
From: Dave Page <dpage@pgadmin.org>
To: Sanket Mehta <sanket.mehta@enterprisedb.com>
Cc: pgadmin-hackers <pgadmin-hackers@postgresql.org>
Content-Type: text/plain; charset=UTF-8
Precedence: bulk
Sender: pgadmin-hackers-owner@postgresql.org

Thanks - committed.

On Wed, Mar 16, 2016 at 7:22 AM, Sanket Mehta
<sanket.mehta@enterprisedb.com> wrote:
> Hi,
>
> I forgot tot mention bugs in previous mail.
>
>
> 1. System cast field was showing Yes even if the cast was not a system cast,
> which I have resolved.
> 2. In Dependent and Dependency method in __init__.py  file, unnecessary
> third parameter 'cast' was being passed which I have removed
>
>
>
> Regards,
> Sanket Mehta
> Sr Software engineer
> Enterprisedb
>
> On Wed, Mar 16, 2016 at 12:43 PM, Sanket Mehta
> <sanket.mehta@enterprisedb.com> wrote:
>>
>> Hi Dave,
>>
>> PFA the patch for cast module incorporating changes regarding showing
>> system objects.
>> Apart from support for showing system object I have resolved few bugs in
>> that, unnecessary code and added nodes.sql file.
>>
>> Please do review it and if it looks good, please commit.
>>
>>
>> Regards,
>> Sanket Mehta
>> Sr Software engineer
>> Enterprisedb
>>
>> On Fri, Mar 4, 2016 at 4:33 PM, Dave Page <dpage@pgadmin.org> wrote:
>>>
>>> Thanks, patch applied.
>>>
>>> On Tue, Mar 1, 2016 at 7:20 AM, Sanket Mehta
>>> <sanket.mehta@enterprisedb.com> wrote:
>>> > Hi,
>>> >
>>> > There was an error in cast module while fetching its dependency and
>>> > dependent.
>>> > Below is the patch which resolves this issue.
>>> > Please review and commit it.
>>> >
>>> >
>>> >
>>> > Regards,
>>> > Sanket Mehta
>>> > Sr Software engineer
>>> > Enterprisedb
>>> >
>>> > On Wed, Feb 24, 2016 at 10:17 PM, Dave Page <dpage@pgadmin.org> wrote:
>>> >>
>>> >> Thanks - committed.
>>> >>
>>> >> On Tue, Feb 23, 2016 at 1:34 PM, Sanket Mehta
>>> >> <sanket.mehta@enterprisedb.com> 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 <dpage@pgadmin.org> 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 <dpage@pgadmin.org>
>>> >>>>> wrote:
>>> >>>>>>
>>> >>>>>> And this time with the attachment...
>>> >>>>>>
>>> >>>>>> On Mon, Feb 15, 2016 at 4:53 PM, Dave Page <dpage@pgadmin.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.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 <dpage@pgadmin.org>
>>> >>>>>>>> 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
>>> >
>>> >
>>>
>>>
>>>
>>> --
>>> 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


-- 
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers