Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1aoG7k-0005n4-5a for pgadmin-hackers@arkaria.postgresql.org; Thu, 07 Apr 2016 20:01:52 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1aoG7j-0001lA-L8 for pgadmin-hackers@arkaria.postgresql.org; Thu, 07 Apr 2016 20:01:51 +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 1aoG7i-0001l3-MH for pgadmin-hackers@postgresql.org; Thu, 07 Apr 2016 20:01:51 +0000 Received: from mail-io0-x235.google.com ([2607:f8b0:4001:c06::235]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1aoG7b-00023Q-0c for pgadmin-hackers@postgresql.org; Thu, 07 Apr 2016 20:01:49 +0000 Received: by mail-io0-x235.google.com with SMTP id o126so84915462iod.0 for ; Thu, 07 Apr 2016 13:01:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin-org.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc; bh=i+XF/AtaRSHj6PdcGVWPAMpWrC4d2nOb+Ezt8nBl9uc=; b=AWXhE2MebObToQCWAKECeF32IiMibMbr5pXmU8eJH1dhjKz12uFokScCQ/Zi05MB1F nJs97weBBeHdO1rcOA3DdL/Af3jexJ8okC4u/ucXCkvmITnUqtoWFukyqLLYsitT4AnY QCTmXwfgFHsQsfZp1P4QN3AAgQKGnBN15RLjs5Kc3I0Xm4U8PAzFq6IjXt+5UW5W8WuT 9wXqQPK0NaqEIVzqXFhTQiv0700NCAu3Nb46OLKGBISCk1BglaOpcEVoOuglKcYbYnLB 4Jb33194RWk3NbZnXOUgSIhkzxMrE3HocoMU/V484/xS05x9ex4ZZeyKMwBIkD1SQY3i 9AAA== 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:date :message-id:subject:from:to:cc; bh=i+XF/AtaRSHj6PdcGVWPAMpWrC4d2nOb+Ezt8nBl9uc=; b=VEmh/2LoPbTUIscN8FIxuGzT/Tq1y2lAoo/UaYqXzJxEj9IGuhz7rgVKnLvKOa9sQb ZeFxybooh8gUBzDNQOHKOnarAfGQFdZkI03nOiRzC7tli9N3Chj4A8qo/zC7js6+akld BxsNJoLNvYurfyuQ+7Evt/91QTmMz3EKBrPi6yA4jxs8AhHab3bqtOVLZqJLSL6zc98C Jtz6zraQ3mR+l+NuQlJVQWS3vGOpnbUOTcMFtWl78wDPOnDYPv1WU4LqWHqM2U6maKY6 qJmId0WGd/mX1lwrA0kCG/Jpm6YeykyeUV9ykiX8mCkIMEHCNA/f4Cs1a7qErufV8s3G lTew== X-Gm-Message-State: AD7BkJKhKoCDAVEsxeQcXpIN7PjZ3LkxFGDQ1/Mlxaw6M6PhAGBwwN2/v3J93ht/rKA6PT68nWtsSC8TACU5Hw== MIME-Version: 1.0 X-Received: by 10.107.12.224 with SMTP id 93mr5557855iom.70.1460059301740; Thu, 07 Apr 2016 13:01:41 -0700 (PDT) Received: by 10.64.105.131 with HTTP; Thu, 7 Apr 2016 13:01:41 -0700 (PDT) In-Reply-To: References: Date: Thu, 7 Apr 2016 21:01:41 +0100 Message-ID: Subject: Re: PATCH: pgadmin 4: FTS Parser From: Dave Page To: Sanket Mehta Cc: Ashesh Vashi , pgadmin-hackers Content-Type: multipart/alternative; boundary=001a113f91ae085150052fea8c9e 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 --001a113f91ae085150052fea8c9e Content-Type: text/plain; charset=UTF-8 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 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.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 > > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --001a113f91ae085150052fea8c9e Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
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, S= anket Mehta <sanket.mehta@enterprisedb.com> wrot= e:
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 r= eview.
Please do review it and provide the comments.
<= /blockquote>
Hi Sanket,

Thanks for the = patch.
Please find my review comments.

<= div>* 'current_app' has been im= ported but not used.
Fixed (it is used now for logging)

* Few variab= les are assigned, but not used further.=C2=A0
=C2=A0 one of the example: "res =3D []" (fts_parser/__init__p= y line#271
Unu= sed variables are removed

* 'gone' is used, bu= t - not imported.
<= div>Fixed

* Do not require __init__(...) function in t= he 'FtsParserModule' class, as it does not do anything here.=
Removed=C2=A0
<= span class=3D"">

* Load the module with the database (not, schema), as it may= be require to show in dependencies list in the database.
Fixed
=C2=A0
=
* = Some of the lines/comments are going beyond the line length limit (i.e. mor= e than 80 character per length).
Fixed=C2=A0

* Please add comments f= or all the methods in=C2=A0FtsParserView.
Fixed=C2=A0

* Do not nee= d 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 wi= thout id=C2=A0=C2=A0for these methods.
=
Fixed=C2=A0

<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;pa= dding-left:1ex">
* Create separate= templates for each methods instead of club them together for the above met= hods.
Ignored = as discussed with Ashesh.=C2=A0

* HTTP method=C2=A0GET= implies for getting/fetching the information/data from the server. Please= =C2=A0remove the 'get_' from the above methods,
<= /div>
Fixed
= =C2=A0
* In= line comments for __init__ method (FtsParserView class) is missing.<= /div>
Fixed=C2=A0

* Do not need to override the 'module_js' method, it ha= s already been implemented in PGChildNodeView class.
Fixed
* Please = fix the correctness of the comments for all the methods. Avoid copy/paste f= rom other modules.
=
Fixed

* Check for the version before setting th= e template_path variable in check_precondition method is not=C2=A0required.
<= /div>
Ignored as per discussion with As= hesh.=C2=A0

* Check the=C2=A0existence=C2=A0of the no= de/object before assuming, it is available (otherwise - return with 'go= ne') in node, and properties method. SQL may not fail, but - no of reco= rds returned will be 0 (zero).
Fixed
=C2=A0
=
* Please test the module on P= ython 3 too.
F= ixed

* Use generate_browser_node from the 'update&= #39; method after successful operation, while generating the result.=
Fixed=C2=A0

* Do not catch exception (if not required) (i.e. in 'upd= ate' method, you will not be able to catch the actual issue in that cas= e). Please remove all unwanted exceptions.
Fixed
=C2=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;pa= dding-left:1ex">
* Log the excepti= on with the application, whenever we catch them.
Fixed
=C2=A0<= /div>
=C2=A0
Note:
I've not yet te= sted the patch.
These are the review comments from the python cod= e 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=C2=A0

--

Thanks & Regards,

Ashesh Vashi
Enterp= riseDB INDIA:=C2=A0Enterprise PostgreSQL Company


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



Regards,
Sanket Mehta
Sr Software engi= neer
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: @pgsna= ke

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Co= mpany
--001a113f91ae085150052fea8c9e--