Re: PATCH: pgadmin 4: FTS Parser

On Mon, Mar 28, 2016 at 2:32 PM, Sanket Mehta <sanket(dot)mehta(at)enterprisedb(dot)com
> wrote:

> Hi,
>
> PFA the patch for FTS parser node for review.
> Please do review it and provide the comments.
>
Hi Sanket,

Thanks for the patch.
Please find my review comments.

* 'current_app' has been imported but not used.
* Few variables are assigned, but not used further.
one of the example: "res = []" (fts_parser/__init__py line#271
* 'gone' is used, but - not imported.
* Do not require __init__(...) function in the 'FtsParserModule' class, as
it does not do anything here.
* Load the module with the database (not, schema), as it may be require to
show in dependencies list in the database.
* Some of the lines/comments are going beyond the line length limit (i.e.
more than 80 character per length).
* Please add comments for all the methods in FtsParserView.
* Do not need the URL routes for get_start, get_token, get_token,
get_headline with id, as it does not use the fts parse id. Declare these
URL-routes without id for these methods.
* Create separate templates for each methods instead of club them together
for the above methods.
* HTTP method GET implies for getting/fetching the information/data from
the server. Please remove the 'get_' from the above methods,
* Inline comments for __init__ method (FtsParserView class) is missing.
* Do not need to override the 'module_js' method, it has already been
implemented in PGChildNodeView class.
* Please fix the correctness of the comments for all the methods. Avoid
copy/paste from other modules.
* Check for the version before setting the template_path variable in
check_precondition method is not required.
* Check the existence of the node/object before assuming, it is available
(otherwise - return with 'gone') in node, and properties method. SQL may
not fail, but - no of records returned will be 0 (zero).
* Please test the module on Python 3 too.
* Use generate_browser_node from the 'update' method after successful
operation, while generating the result.
* Do not catch exception (if not required) (i.e. in 'update' method, you
will not be able to catch the actual issue in that case). Please remove all
unwanted exceptions.
* Log the exception with the application, whenever we catch them.

Note:
I've not yet tested the patch.
These are the review comments from the python code only.
You may also want to look at the javascript module before sending for
review again.
(i.e. code should be wrapped after the line #79.)

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company
<http://www.enterprisedb.com/>

*http://www.linkedin.com/in/asheshvashi*
<http://www.linkedin.com/in/asheshvashi>

> Regards,
> Sanket Mehta
> Sr Software engineer
> Enterprisedb
>
>
> --
> Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers
>
>