Re: patch for cast module
От | Dave Page |
---|---|
Тема | Re: patch for cast module |
Дата | |
Msg-id | CA+OCxoxWad42aBeEUTDGbMSPO_GMbkYc=weGdt6JyDoy-ipc4w@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: patch for cast module (Sanket Mehta <sanket.mehta@enterprisedb.com>) |
Список | pgadmin-hackers |
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
В списке pgadmin-hackers по дате отправления: