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 1fT7eF-0001uQ-D2 for pgadmin-hackers@arkaria.postgresql.org; Wed, 13 Jun 2018 15:25:23 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fT7eE-00029c-41 for pgadmin-hackers@arkaria.postgresql.org; Wed, 13 Jun 2018 15:25:22 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fT7eD-00029P-Re for pgadmin-hackers@lists.postgresql.org; Wed, 13 Jun 2018 15:25:22 +0000 Received: from mail-wr0-x241.google.com ([2a00:1450:400c:c0c::241]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fT7eA-0001Ly-A6 for pgadmin-hackers@postgresql.org; Wed, 13 Jun 2018 15:25:21 +0000 Received: by mail-wr0-x241.google.com with SMTP id l10-v6so3221365wrn.2 for ; Wed, 13 Jun 2018 08:25:17 -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:from:date:message-id:subject:to :cc; bh=ne5Xi5qrdhdbjN2qJ0ASMMb8Q7BzVwwHoPlMFbz2/wo=; b=S6zeHwk0AFdzbmdeU+q1TlQtIWCun+32P6c1dWNU5SFZskYChF7mKEc34WruFh0Uno XsqJFUPYF+T2L9Afa3S6luOphPIcLBVG7LHiu1fThzOlvG5sm1cgch2Y0KXAsPzdZMR2 x/pKfmL9i/hRLAEAmEsk6GCDOc3FNTnJGBdDdT7+fxBPQZ7mYGaJ8BK345waDGZk0pQw i7BEY5Is+06LrBjXAUl3+DAt9BZtE+wsoI1io9g0JNt2NLSYG8kVZ+ymHJBmqShzI3k6 Ha6iYOYIJG++HvgD6738r8nCyGwHvPSeiPkXxsKdCOwLXOGUGLbVRlM4mIer146lMKGe X+IA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=ne5Xi5qrdhdbjN2qJ0ASMMb8Q7BzVwwHoPlMFbz2/wo=; b=I3BtvjusbqT+StXXeogW0xJOcFPCyW/luwhVk8pG664h24MzCQObb21CxFmf/HsYIp eJODSe6wMQi6eycYv8jDdQmeUb5Wm8R2AQNIZb8i8Y5p1SJfYVpCORxYlTvQIRQ4kIo8 d8i8dWe4M+0c0y1bCal0N0hqI3RcSYpAhYruOA0+BAg0DCpMISKu/RvpnSb2sDH3hgdw mNkzve0RgOGyFRmQJIsNvDrK9/Tt8RoG59lDkWR+Z7Fotxc1IASav54C0KtfhZ6uEQ5v BM9U+F/8er1vRvrJkCp4OaF4eDWNNzipxlpunNsrnni9WO8bZwFgeD4wHhs+2Pw/LuKW 9N4A== X-Gm-Message-State: APt69E2xfSZm58/LAFIRYZfVARNQBsf2KcO4WzNyrzaByV94v9lLzWYt r9Mz99P6QX95B+3dVbYyXjaPc1n9v7dB12+8VcVVfw== X-Google-Smtp-Source: ADUXVKJFFOG/8s4K9WYvnS/WO/vGSTnLcZOAj/SeE+Nx4RclLAyjiOUXEn9ZfDk1ChTMvnlkIl/JSYDSsLCZDMDSKR0= X-Received: by 2002:adf:8211:: with SMTP id 17-v6mr4536838wrb.144.1528903517404; Wed, 13 Jun 2018 08:25:17 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a1c:2907:0:0:0:0:0 with HTTP; Wed, 13 Jun 2018 08:25:15 -0700 (PDT) In-Reply-To: References: From: Dave Page Date: Wed, 13 Jun 2018 16:25:15 +0100 Message-ID: Subject: Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, and add support procedure for PG v11 To: Victoria Henry Cc: Khushboo Vashi , pgadmin-hackers Content-Type: multipart/alternative; boundary="0000000000000d09ea056e8797ab" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000000d09ea056e8797ab Content-Type: text/plain; charset="UTF-8" On Wed, Jun 13, 2018 at 4:00 PM, Victoria Henry wrote: > 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 == '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. > The responsibility here is to handle the objects from pg_proc which are fundamentally almost identical bar a couple of nuances in the SQL spec. Looking at the code you quote (or at least that I think you're quoting), I see there's maybe duplication of 50% - 75% or so of the code in each branch which probably could be cleaned up. > 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. > Don't mistake not spotting an opportunity with intentionally ignoring it. > > Dave, we don't understand why this change was committed into master, when > there was a clear disagreement. > I must be missing an email then. I only saw one from you previously, to which Khushboo addressed all the concerns you raised. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --0000000000000d09ea056e8797ab Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Wed, Jun 13, 2018 at 4:00 PM, Victoria Henry <<= a href=3D"mailto:vhenry@pivotal.io" target=3D"_blank">vhenry@pivotal.io= > wrote:
Hi,
Khushboo, thanks for making some of the changes we mentio= ned 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 b= etween them. This breaks the Single Responsibility Principle, that says tha= t each class should have only one responsibility on the context of the appl= ication.

The responsibility here is to handle the objects from pg_proc which ar= e fundamentally almost identical bar a couple of nuances in the SQL spec. L= ooking at the code you quote (or at least that I think you're quoting),= I see there's maybe duplication of 50% - 75% or so of the code in each= branch which probably could be cleaned up.
=C2=A0
Even if we completel= y ignore SOLID principles the clear opportunity for a refactor that would m= ake the code much easier to read was lost, when last Monday we agreed that = we should not lose these opportunities.

Don't mistake not spotting an opportunity with intentionally i= gnoring it.
=C2=A0

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

I must be missing an email then. I o= nly saw one from you previously, to which Khushboo addressed all the concer= ns you raised.=C2=A0

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

EnterpriseDB UK: http://www.enterprisedb.com
The Ente= rprise PostgreSQL Company
--0000000000000d09ea056e8797ab--