Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1amzLQ-0002Sy-VL for pgadmin-hackers@arkaria.postgresql.org; Mon, 04 Apr 2016 07:54:45 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1amzLQ-0003L4-H2 for pgadmin-hackers@arkaria.postgresql.org; Mon, 04 Apr 2016 07:54:44 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1amzLC-0002oS-7M for pgadmin-hackers@postgresql.org; Mon, 04 Apr 2016 07:54:30 +0000 Received: from mail-ig0-x231.google.com ([2607:f8b0:4001:c05::231]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1amzL5-0006EU-Gw for pgadmin-hackers@postgresql.org; Mon, 04 Apr 2016 07:54:29 +0000 Received: by mail-ig0-x231.google.com with SMTP id gy3so17939581igb.1 for ; Mon, 04 Apr 2016 00:54:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=BLK0/LRJ1KKzXL72oNT9uu1Hp6BG7znGidlytO3EpJA=; b=DNejO84wYky7DPPuII3543RSCz6/g1E3yosb20pB45VAlk9APK2KuIQMKYK475q79D fi410DRcCutEfmKwsiLIDZc1AhqNJcXqWLuC7C2StO82v9dYB9a6+/DnLKNDCMrYhX2Y xWE7SJ2MeVZSI/zEUKFeRnT4AqPcoaXo0/XvDH6kBLlnMMRRJegCjXlNXOvB165z4UCL 2jnhhSsmVLf4c3AiAJKl0JalIhveR9IqlIy5269g3GI87Qx7rJtN+QqmnQMLdUCK7n2N 5dVeCdXaVtk2V/Yl82fGd6gQnm8lIIQD/zSe7g0P3WHjqSCxrqzNo4jPd+0RUVVh57lN ZDrQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=BLK0/LRJ1KKzXL72oNT9uu1Hp6BG7znGidlytO3EpJA=; b=hqCzvrtpYWiK3k99l0SqesoU3l+5wWfTQB4eqLKmfSS3E72I6Ww0Q/r/H65yToRyEL 1zjcEFt1evuVaUKuGF8I6Ec+RJR2wuEUflGZRpmw30MAPkUk0dJJlNn+rmWLCYVUfhVS P7gn6RBx6Fa6OezvogLx+S5iOT5rooWdRT+v+dZ0s4qqvSHIX/1jI7YtyyKdaxItjS2s YFG7rrIeMKKYg8nWEdV3UnztLrm9vTjLOd2jhvs5/ElXi5CEG1HoghEYCbGN6iP8O6x8 1vvYGmu0ziF2dj85Auqr8JGtalcpaecG+tHWHDOjVoSJGx0rJEZpA52JmfTuoR4+S27A v2hA== X-Gm-Message-State: AD7BkJIWpsFPJfiQe4anuvPU2p0ym4Jay4c6okRNYIpy9L0C2W6yQrNGSQmC3CRUTEiF4TVFfZ8bx/gl8pqnSxQt X-Received: by 10.50.143.102 with SMTP id sd6mr4442302igb.73.1459756462470; Mon, 04 Apr 2016 00:54:22 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.39.5 with HTTP; Mon, 4 Apr 2016 00:54:03 -0700 (PDT) In-Reply-To: References: From: Ashesh Vashi Date: Mon, 4 Apr 2016 13:24:03 +0530 Message-ID: Subject: Re: PATCH: pgadmin 4: FTS Parser To: Sanket Mehta Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary=001a1135e45667b239052fa4092c X-Pg-Spam-Score: -2.6 (--) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgadmin-hackers Precedence: bulk Sender: pgadmin-hackers-owner@postgresql.org --001a1135e45667b239052fa4092c Content-Type: text/plain; charset=UTF-8 On Mon, Mar 28, 2016 at 2:32 PM, Sanket Mehta 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.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 > > --001a1135e45667b239052fa4092c Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

On Mon, Mar 28, 2016 at 2:32 PM, Sanket Mehta <sanket.meh= ta@enterprisedb.com> wrote:

Hi,

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

Thanks for the patch.
Please find my revi= ew comments.

* 'current_app' has been imported but not used.
* Few variables are assigned, but not= used further.
=C2=A0 = one of the example: "res =3D []" (fts_parser/__init__py line#271<= /font>
* 'gone' is us= ed, but - not imported.
* 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 goi= ng beyond the line length limit (i.e. more than 80 character per length).
* Please add comments f= or all the methods in=C2=A0FtsParserView.
* Do not need the URL routes for get_start, get_token, = get_token, get_headline with id,=C2=A0as it does not use the fts parse id.= =C2=A0Declare these URL-routes without id=C2=A0=C2=A0for these methods.
* Create separate templat= es for each methods instead of club them together for the above methods.
* HTTP method=C2=A0GET i= mplies for getting/fetching the information/data from the server. Please=C2= =A0remove the 'get_' from the above methods,
* Inline comments for __init__ method (FtsPa= rserView class) is missing.
* Do not need to override the 'module_js' method, it has alre= ady 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=C2=A0existence= =C2=A0of the node/object before assuming, it is available (otherwise - retu= rn with 'gone') in node, and properties method. SQL may not fail, b= ut - no of records returned will be 0 (zero).
* Please test the module on Python 3 too.<= /div>
* Use generate_browser_node f= rom the 'update' method after successful operation, while generatin= g the result.
* Do not= catch exception (if not required) (i.e. in 'update' method, you wi= ll not be able to catch the actual issue in that case). Please remove all u= nwanted exceptions.
* = Log the exception with the application, whenever we catch them.
=C2=A0
Note:
I've not yet tested the pa= tch.
These are the review comments from the python code only.
You may also want to look at the javascript module before sending fo= r review again.
(i.e. code should be wrapped after the line #79.)=

--

Thanks & Regar= ds,

<= span style=3D"font-style:italic">Ashesh Vashi
EnterpriseDB INDIA:=C2=A0= Enterprise Postg= reSQL Company


http://www.linked= in.com/in/asheshvashi


=

Regards,Sanket Mehta
Sr Software engineer
Enterprisedb
<= /div>


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


--001a1135e45667b239052fa4092c--