Re: patch for cast module

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Dave Page <[email protected]>
To: Sanket Mehta <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: patch for cast module
Date: Fri, 4 Mar 2016 11:03:41 +0000
Message-ID: <CA+OCxoyAMmG88zNTGSLjdLpomwc9JHksULQQD80xrr3b8WZLqA@mail.gmail.com> (raw)
In-Reply-To: <CA+yw=mO9QjYBPHBNeJSr3TC-iLxKL1AuwFuvMWCCj-XnQTSsGQ@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>
List-Unsubscribe:  <mailto:[email protected]?body=unsub%20pgadmin-hackers>

Thanks, patch applied.

On Tue, Mar 1, 2016 at 7:20 AM, Sanket Mehta
<[email protected]> 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 <[email protected]> wrote:
>>
>> Thanks - committed.
>>
>> On Tue, Feb 23, 2016 at 1:34 PM, Sanket Mehta
>> <[email protected]> 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 <[email protected]> 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
>>>> <[email protected]> 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 <[email protected]> wrote:
>>>>>>
>>>>>> And this time with the attachment...
>>>>>>
>>>>>> On Mon, Feb 15, 2016 at 4:53 PM, Dave Page <[email protected]> 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
>>>>>>> <[email protected]> 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 <[email protected]>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Mon, Feb 15, 2016 at 8:10 AM, Sanket Mehta
>>>>>>>>> <[email protected]> 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


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

view thread (24+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: patch for cast module
  In-Reply-To: <CA+OCxoyAMmG88zNTGSLjdLpomwc9JHksULQQD80xrr3b8WZLqA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox