Thanks - committed with one minor bug: - Renaming a parser doesn't update the label on the treeview node.
I've added a card to our internal kanban chart for this - please submit a patch to fix ASAP. I also fixed some minor string and SQL related issues. On Thu, Apr 7, 2016 at 3:27 PM, Sanket Mehta <sanket.me...@enterprisedb.com> wrote: > 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.va...@enterprisedb.com> wrote: > >> On Mon, Mar 28, 2016 at 2:32 PM, Sanket Mehta < >> sanket.me...@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.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@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 > > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
