Re: PATCH: pgadmin 4: FTS Parser

Hi,

PFA the revised patch.

response is inline.

Please do review the patch and revert with comments if any.

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

On Mon, Apr 4, 2016 at 1:24 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:

On Mon, Mar 28, 2016 at 2:32 PM, Sanket Mehta <sanket.mehta@enterprisedb.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.

Fixed (it is used now for logging)

* Few variables are assigned, but not used further. 

  one of the example: "res = []" (fts_parser/__init__py line#271

Unused variables are removed

* 'gone' is used, but - not imported.

Fixed

* Do not require __init__(...) function in the 'FtsParserModule' class, as it does not do anything here.

Removed 

* Load the module with the database (not, schema), as it may be require to show in dependencies list in the database.

Fixed

 

* Some of the lines/comments are going beyond the line length limit (i.e. more than 80 character per length).

Fixed 

* Please add comments for all the methods in FtsParserView.

Fixed 

* 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.

Fixed 

* Create separate templates for each methods instead of club them together for the above methods.

Ignored as discussed with Ashesh. 

* HTTP method GET implies for getting/fetching the information/data from the server. Please remove the 'get_' from the above methods,

Fixed

 

* Inline comments for __init__ method (FtsParserView class) is missing.

Fixed 

* Do not need to override the 'module_js' method, it has already been implemented in PGChildNodeView class.

Fixed

* Please fix the correctness of the comments for all the methods. Avoid copy/paste from other modules.

Fixed

* Check for the version before setting the template_path variable in check_precondition method is not required.

Ignored as per discussion with Ashesh. 

* 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).

Fixed

 

* Please test the module on Python 3 too.

Fixed

* Use generate_browser_node from the 'update' method after successful operation, while generating the result.

Fixed 

* 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.

Fixed

 

* Log the exception with the application, whenever we catch them.

Fixed

 

 

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.)

Fixed 

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company

http://www.linkedin.com/in/asheshvashi

Regards,

Sanket Mehta

Sr Software engineer

Enterprisedb

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers

--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers