public inbox for [email protected]  

help / color / mirror / Atom feed

From: 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 13:39:43 +0000
Message-ID: <CA+OCxozy4aH9=2T0Sp_WworU_3UQaaHBcQXTDhEUKHAkZJT8ow@mail.gmail.com> (raw)
In-Reply-To: <CAM5-9D8oYyEaL89coD9FozPDziGhYutYAat9dMP9dAwhOp5OKQ@mail.gmail.com>
References: <CAM5-9D8oYyEaL89coD9FozPDziGhYutYAat9dMP9dAwhOp5OKQ@mail.gmail.com>
List-Unsubscribe:  <mailto:[email protected]?body=unsub%20pgadmin-hackers>

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

- 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

Thanks.

-- 
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+OCxozy4aH9=2T0Sp_WworU_3UQaaHBcQXTDhEUKHAkZJT8ow@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