public inbox for [email protected]
help / color / mirror / Atom feedFrom: Dave Page <[email protected]>
To: Surinder Kumar <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: [pgAdmin4] [Patch]: Grant Wizard
Date: Fri, 4 Mar 2016 14:36:09 +0000
Message-ID: <CA+OCxoyGKAorswp0fwUojT4gB0Ex+dcRxzKbLkfG48qeCmSCrQ@mail.gmail.com> (raw)
In-Reply-To: <CA+OCxozy4aH9=2T0Sp_WworU_3UQaaHBcQXTDhEUKHAkZJT8ow@mail.gmail.com>
References: <CAM5-9D8oYyEaL89coD9FozPDziGhYutYAat9dMP9dAwhOp5OKQ@mail.gmail.com>
<CA+OCxozy4aH9=2T0Sp_WworU_3UQaaHBcQXTDhEUKHAkZJT8ow@mail.gmail.com>
List-Unsubscribe: <mailto:[email protected]?body=unsub%20pgadmin-hackers>
A couple of corrections below <sigh>
On Fri, Mar 4, 2016 at 1:39 PM, Dave Page <[email protected]> wrote:
> Hi
>
> On Thu, Mar 3, 2016 at 12:49 PM, Surinder Kumar
> <[email protected]> wrote:
>> Hi,
>>
>> PFA patch for Grant Wizard.
>>
>> Before applying grant wizard patch:
>>
>> 1. Apply patch for "wizard JS file" which Khushboo had shared with
>> Ashesh personally.
>> I am using that patch with few changes in that. Ashesh will
>> review
>> and commit that patch.
>>
>> 2. Apply patches of "Sequence" and "Functions" macros.
>>
>>
>> Please review the patch and Let me know for any comments.
>
> Initial feedback:
>
> - Why does this add specific knowledge of the Grant Wizard to the
> Browser module? It should be a plugin that loads itself at runtime.
> Any changes to the browser to support that should be entirely generic
> and usable by any module.
>
> - The comment above also applies to the core templates. CSS should be
> advertised by the plugin, and the browser can add it into the rendered
> output when the module is dynamically loaded.
>
> - +""" Implements Grant Wizard""" - why the leading space? Please
> review all comments and code for such small details.
>
> - The Python code to detect and alias various Python 2/3 classes
> should be centralised, as I've seen it in at least one other module.
>
> - In other module names, we've separated multiple words with a hypen.
> e.g. server-groups. s/grantwizard/grant-wizard/g
That should be an underscore, not a hyphen:
s/grantwizard/grant_wizard/g
>
> - The published routes for this module are all under
>
> - "GET /browser/static/js/wizard.js HTTP/1.1" 404 -
>
> - For consistency, when naming CSS/JS files that are core to a module,
> please use the module name, e.g.
> /web/pgadmin/tools/grant-wizard/static/css/grant-wizard.css
/web/pgadmin/tools/grant_wizard/static/css/grant_wizard.css
--
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: [pgAdmin4] [Patch]: Grant Wizard
In-Reply-To: <CA+OCxoyGKAorswp0fwUojT4gB0Ex+dcRxzKbLkfG48qeCmSCrQ@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