A couple of corrections below <sigh> On Fri, Mar 4, 2016 at 1:39 PM, Dave Page <dp...@pgadmin.org> wrote: > Hi > > On Thu, Mar 3, 2016 at 12:49 PM, Surinder Kumar > <surinder.ku...@enterprisedb.com> 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 (pgadmin-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers