Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.80) (envelope-from ) id 1abnWf-0004u2-7T for pgadmin-hackers@arkaria.postgresql.org; Fri, 04 Mar 2016 11:04:05 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84) (envelope-from ) id 1abnWe-0004BF-QW for pgadmin-hackers@arkaria.postgresql.org; Fri, 04 Mar 2016 11:04:04 +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 1abnWQ-0003mY-EB for pgadmin-hackers@postgresql.org; Fri, 04 Mar 2016 11:03:50 +0000 Received: from mail-ig0-x235.google.com ([2607:f8b0:4001:c05::235]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84) (envelope-from ) id 1abnWJ-00067w-8k for pgadmin-hackers@postgresql.org; Fri, 04 Mar 2016 11:03:49 +0000 Received: by mail-ig0-x235.google.com with SMTP id y8so8497283igp.0 for ; Fri, 04 Mar 2016 03:03:42 -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; bh=B+KIbLbr230cg/ZsamKrR5RzUjZlUYhFupC99Jxsivg=; b=sqOo6Kjqq6RLCOEFdMFg4s9w/xSeOwuBIFodBDgG4Ew50L7ZfARcahaueHKEm7kxDp EHAsVSrknCtBIZgEdfSG/GPnFjsheTR9iK0TVLE/GY9641HPFJYbo8hmgG9LohCN8uAA hAsx4EgOT0JWu8E5n70AXJmvAx7s1ZbjyUCQc2YDd5rNpd1KOCKmZJ9/ueRtAcGExfLt go0IuFJw1b0bPFTPnsKwjLG9ZawnRFTFjrIqywnGPVYDCjTIilSIml/8gOD26biL6ECP oHxMePNGhGbu5ZZExdGa4Uz6lldodQkMhqvMtCAMsUrOkU7vy30uovBZ63KySTJ9p/wD yNCA== 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; bh=B+KIbLbr230cg/ZsamKrR5RzUjZlUYhFupC99Jxsivg=; b=erPrqoSO1F9ifo/zxMDbo5Aae7wi3m2uGCrMlAZ+gEhnXfRENe1qDG2QHBBOhR2art Ij7UFF2ZIZLJUjDwYF7hVi9FygW9iHr3VkBVSWPePtaxMK8skSKe8t8mtaLpK87/mPEZ XGA7aU3/T9ZVAx+ki+vWhJ/KZ+PMq8l3K+az/eoOjI1/RHUMPx+gMGrJKJruk1FzQ0HH Q1tOYWhVsqo1vll5IZ6PJHWEQDToP8WmajRFu42FlhkK+0/Pj2adMUHPZxdzErb1A8qZ Oiv3YsetWm1uB8Mvxq8kjXVXZjgpPPJ4tdDWWrkJDFqhpmmYCW3XxaDhxvnXYQv2v2T6 /Chw== X-Gm-Message-State: AD7BkJK6TTLXHhu14TlyLOCVtIZAgFn5MloMo8n4NH+eebMSurmE48zTcsknhLjN4Xp87jeDnOoII9engDgIJQ== MIME-Version: 1.0 X-Received: by 10.50.134.129 with SMTP id pk1mr3036720igb.11.1457089421731; Fri, 04 Mar 2016 03:03:41 -0800 (PST) Received: by 10.64.90.3 with HTTP; Fri, 4 Mar 2016 03:03:41 -0800 (PST) In-Reply-To: References: Date: Fri, 4 Mar 2016 11:03:41 +0000 Message-ID: Subject: Re: patch for cast module From: Dave Page To: Sanket Mehta Cc: pgadmin-hackers Content-Type: text/plain; charset=UTF-8 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 Thanks, patch applied. On Tue, Mar 1, 2016 at 7:20 AM, Sanket Mehta 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 wrote: >> >> 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 >>>> 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 >>>>>>> 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 >>>>>>>>> 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 (pgadmin-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers