Re: PATCH: pgadmin 4: FTS Parser

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(dot)mehta(at)enterprisedb(dot)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(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>> 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.
>>
> 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(at)postgresql(dot)org)
>>> To make changes to your subscription:
>>> http://www.postgresql.org/mailpref/pgadmin-hackers
>>>
>>>
>>
>
>
> --
> 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
>
>

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company