Received: from malur.postgresql.org ([2a02:16a8:dc51::56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fT7Gk-0008Of-J7 for pgadmin-hackers@arkaria.postgresql.org; Wed, 13 Jun 2018 15:01:06 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fT7Gj-00008F-2s for pgadmin-hackers@arkaria.postgresql.org; Wed, 13 Jun 2018 15:01:05 +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.89) (envelope-from ) id 1fT7Gi-000084-Kc for pgadmin-hackers@lists.postgresql.org; Wed, 13 Jun 2018 15:01:04 +0000 Received: from mx0b-00296801.pphosted.com ([148.163.153.148]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fT7GZ-0004a3-W6 for pgadmin-hackers@postgresql.org; Wed, 13 Jun 2018 15:01:02 +0000 Received: from pps.filterd (m0114584.ppops.net [127.0.0.1]) by mx0b-00296801.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w5DF0sEj016217 for ; Wed, 13 Jun 2018 15:00:54 GMT Received: from mail-it0-f72.google.com (mail-it0-f72.google.com [209.85.214.72]) by mx0b-00296801.pphosted.com with ESMTP id 2jjp780kbr-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Wed, 13 Jun 2018 15:00:54 +0000 Received: by mail-it0-f72.google.com with SMTP id n66-v6so5845968itg.0 for ; Wed, 13 Jun 2018 08:00:54 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=JtpNoBkMQuBcuU+Gu2WSeGWPHNCyOnHshnZ5tmNUgDU=; b=Scm6MgLQyJziAV/JBhRD5Pl+lRj2bVdykyJpjajamy4pHwoK25XyIkhFikTgKk2jZk 5oiwRJv/2pQzLpeDixvCquaYvLknpOuYPlQzETffRc2YaikwrEtiB6vu5dhlB6RIUdOc UEvKCrnJ22CVFfbjqpt5eGMjtkC/VfiN0qPX/izdZOQD+vU5o0PvG9J2C7Gbkwuayatq NADXtnKbKR8ewdLgeXczvnwWBNViqojvYw2NCB8GSeNRnqWAkMKIB0HMqcp73+MaUvbD 3nkZZf1JfqT4BfYQl7bbt1JgfZsArvzD6M9QVMbKGUkCaR3Jm5SuHyQIxOrzcvSTPKDu l1+w== X-Gm-Message-State: APt69E1AZTGLUJbydMnq4F4Tb2LVrG+vhKY1gX69bi7VpECYphJrNrDH rZm76RPgtAawUei+3sPUc2ZLUuoQBYFtmiRqS3p3l7iBHL19CEMALiOCOjTTYurtZJ8FpsX4Bdx adCdVblqeEDhDE/g2kjU8rx4sQt9wZ0N4KTwg7+V5nFZo2LlctejfT2ZpDRbHN4rzRpEF X-Received: by 2002:a6b:ea09:: with SMTP id m9-v6mr5267107ioc.121.1528902052494; Wed, 13 Jun 2018 08:00:52 -0700 (PDT) X-Google-Smtp-Source: ADUXVKKOjx06yBSLweBe2FestSOI/d2832MhQBVSLNyGHkvcrhRax7R+YSuHMgDODXioIZezrEOLA+d6gAxUHVdWMJw= X-Received: by 2002:a6b:ea09:: with SMTP id m9-v6mr5267067ioc.121.1528902052125; Wed, 13 Jun 2018 08:00:52 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Victoria Henry Date: Wed, 13 Jun 2018 11:00:41 -0400 Message-ID: Subject: Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, and add support procedure for PG v11 To: Dave Page Cc: Khushboo Vashi , pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000b6b715056e873f05" X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-06-13_06:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=2 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1805220000 definitions=main-1806130162 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000b6b715056e873f05 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi, Khushboo, thanks for making some of the changes we mentioned in our previous email. But when you say that they share logic and the code is if self.node_type =3D=3D 'procedure': 50+ lines of code else: 50+ lines of code This tells us that they share some logic but there is a huge difference between them. This breaks the Single Responsibility Principle, that says that each class should have only one responsibility on the context of the application. Even if we completely ignore SOLID principles the clear opportunity for a refactor that would make the code much easier to read was lost, when last Monday we agreed that we should not lose these opportunities. Dave, we don't understand why this change was committed into master, when there was a clear disagreement. Sincerely, Victoria && Joao On Wed, Jun 13, 2018 at 10:03 AM Dave Page wrote: > Thanks, patch applied. > > On Wed, Jun 13, 2018 at 12:43 PM, Khushboo Vashi < > khushboo.vashi@enterprisedb.com> wrote: > >> Please ignore my previous patch. Find the attached updated patch. >> >> On Wed, Jun 13, 2018 at 9:32 AM, Khushboo Vashi < >> khushboo.vashi@enterprisedb.com> wrote: >> >>> Hi Victoria, >>> >>> The updated patch is attached. >>> >>> On Tue, Jun 12, 2018 at 9:36 PM, Victoria Henry >>> wrote: >>> >>>> Hi Khushboo, >>>> >>>> The following change is allowing the creation of procedures in >>>> postgresql versions less then 11 and also GreenPlum >>>> >>>> --- a/web/pgadmin/browser/server_groups/servers/databases/schemas/func= tions/static/js/procedure.js >>>> +++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/func= tions/static/js/procedure.js >>>> @@ -71,11 +71,7 @@ define('pgadmin.node.procedure', [ >>>> if ('catalog' in node_hierarchy) >>>> return false; >>>> >>>> - // Procedures supported only in PPAS >>>> - return ( >>>> - 'server' in node_hierarchy && >>>> - node_hierarchy['server'].server_type =3D=3D 'ppas' >>>> - ); >>>> + return true; >>>> >>>> Fixed. >>> >>>> Now that the Procedures are a thing in Postgresql maybe they should >>>> live in their own module. >>>> >>> The main functionalities of the functions and procedures are almost sam= e >>> and we have inherited most of the things from function itself. >>> So, as per me they should live in one module. >>> >>>> In the tests for trigger functions we are not consistent on the naming >>>> of the utils , in some places we call it funcs_utils in others >>>> trigger_funcs_utils. >>>> >>> Fixed. >>> >>>> Thanks >>>> =E2=80=8B >>>> Victoria & Joao >>>> >>>> Thanks, >>> Khushboo >>> >>>> On Tue, Jun 12, 2018 at 3:10 AM Khushboo Vashi < >>>> khushboo.vashi@enterprisedb.com> wrote: >>>> >>>>> Hi, >>>>> >>>>> Please find the attached updated patch. >>>>> >>>>> On Fri, Jun 8, 2018 at 2:21 PM, Dave Page wrote: >>>>> >>>>>> Hi >>>>>> >>>>>> On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi < >>>>>> khushboo.vashi@enterprisedb.com> wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> Please ignore my previous patch, find the attached updated one. >>>>>>> >>>>>> >>>>>> I found a couple of issues with this: >>>>>> >>>>>> - Clicking the + button on the Parameters tab does nothing in either >>>>>> Create or Edit modes >>>>>> >>>>>> Fixed >>>>> >>>>>> - The debugger fails to start (though, perhaps that's because the >>>>>> plugin doesn't have Ashesh's latest patches in it). >>>>>> >>>>>> FYI, I was trying to test the debugger with: >>>>>> >>>>>> -- PROCEDURE: public.dummy_proc(integer) >>>>>> >>>>>> -- DROP PROCEDURE public.dummy_proc(integer); >>>>>> >>>>>> CREATE OR REPLACE PROCEDURE public.dummy_proc( >>>>>> id integer) >>>>>> LANGUAGE 'plpgsql' >>>>>> >>>>>> AS $BODY$BEGIN >>>>>> raise notice 'id is %', id; >>>>>> END;$BODY$; >>>>>> >>>>>> Fixed. Tested with the latest code of the plugin. >>>>> >>>>>> Thanks! >>>>>> >>>>>> Thanks, >>>>> Khushboo >>>>> >>>>>> -- >>>>>> Dave Page >>>>>> Blog: http://pgsnake.blogspot.com >>>>>> Twitter: @pgsnake >>>>>> >>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>> The Enterprise PostgreSQL Company >>>>>> >>>>> >>>>> >>> >> > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > --000000000000b6b715056e873f05 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi,

Khushboo, thanks for making some of= the changes we mentioned in our previous email.
But when you say= that they share logic and the code is

if self.node_type =3D=
=3D 'procedure':
= 50+ lines of code
else:
50+ lines of code<= /div>

This tells us that they share some logic but there= is a huge difference between them. This breaks the Single Responsibility P= rinciple, that says that each class should have only one responsibility on = the context of the application. Even if we completely ignore SOLID principl= es the clear opportunity for a refactor that would make the code much easie= r to read was lost, when last Monday we agreed that we should not lose thes= e opportunities.

Dave, we don't understand why= this change was committed into master, when there was a clear disagreement= .=C2=A0


Sincerely,

Victoria && Joao

On Wed, Jun 13, 2018 at 10:03 AM Dave Page <dpage@pgadmin.org> wrote:
Thanks, patch applied.

On Wed, Jun 13, 2018 a= t 12:43 PM, Khushboo Vashi <khushboo.vashi@enterprisedb.com<= /a>> wrote:
Pl= ease ignore my previous patch. Find the attached updated patch.

On Wed, Jun 13, = 2018 at 9:32 AM, Khushboo Vashi <khushboo.vashi@enterprisedb= .com> wrote:
Hi Victoria,

The updated patch is attached.
<= div class=3D"gmail_extra">
On Tue, Jun = 12, 2018 at 9:36 PM, Victoria Henry <vhenry@pivotal.io> wrot= e:

Hi Khushboo,

The following change is allowin= g the creation of procedures in postgresql versions less then 11 and also G= reenPlum

--- a/web/pgadmin/br=
owser/server_groups/servers/databases/schemas/functions/static/js/procedure=
.js
+++ b/web/pgadmin/browser/server_groups/servers/databases/schemas/functions=
/static/js/procedure.js
@@ -71,11 +71,7 @@ define('pgadmin.node.procedure', [
         if ('catalog' in node_hierarchy)
           return false;

-        // Procedures supported only in PPAS
-        return (
-          'server' in node_hierarchy &&
-            node_hierarchy['server'].server_type =3D=3D 'ppas&=
#39;
-        );
+        return true;

Fixed.=C2=A0

Now that the Procedures are a thing in = Postgresql maybe they should live in their own module.=C2=A0

The main functionalities of the functions and pro= cedures are almost same and we have inherited most of the things from funct= ion itself.=C2=A0
So, as per me they should live in one module.

I= n the tests for trigger functions we are not consistent on the naming of th= e utils , in some places we call it funcs_utils in = others trigger_funcs_utils.

Fixed.=C2=A0

Thanks

=E2=80=8B
Victori= a & Joao

Thanks,
Khushboo=C2=A0
On Tue, Jun 12, 2018 at 3:10= AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
Hi,

=
Please find the attached updated patch.
=
On Fri, Jun 8, 2018 at 2:21 PM, Dave Page <= dpage@pgadmin.org> wrote:
<= div dir=3D"ltr">Hi

On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <= khushboo.vashi@enterprisedb.com> wrote:
Hi,

<= span>
Please ignore my previous patch, find the attached updated one.

I found a couple of issue= s with this:

- Clicking the + button on the Parame= ters tab does nothing in either Create or Edit modes

Fixed=C2=A0
- The debugger fails to start (though, perhaps that'= s because the plugin doesn't have Ashesh's latest patches in it).

FYI, I was trying to test the debugger with:
<= div>
-- PROCEDURE: public.dummy_proc(integer)
=
-- DROP PROCEDURE public.dummy_proc(integer);

=
CREATE OR REPLACE=C2=A0 PROCEDURE public.dummy_proc(
<= span style=3D"white-space:pre-wrap"> id integer)
LANGUAGE = 'plpgsql'

AS $BODY$BEGIN
=C2=A0 = raise notice 'id is %', id;
END;$BODY$;
<= br>
Fixed. Tested with the latest = code of the plugin.
Thanks!=

= Thanks,
Khushboo=C2=A0
--
Dave = Page
Blog: htt= p://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: <= a href=3D"http://www.enterprisedb.com" target=3D"_blank">http://www.enterpr= isedb.com
The Enterprise PostgreSQL Company






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

E= nterpriseDB UK: h= ttp://www.enterprisedb.com
The Enterprise PostgreSQL Company
--000000000000b6b715056e873f05--