Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, and add support procedure for PG v11

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. 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 <dpage(at)pgadmin(dot)org> wrote:

> Thanks, patch applied.
>
> On Wed, Jun 13, 2018 at 12:43 PM, Khushboo Vashi <
> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>> Please ignore my previous patch. Find the attached updated patch.
>>
>> On Wed, Jun 13, 2018 at 9:32 AM, Khushboo Vashi <
>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Victoria,
>>>
>>> The updated patch is attached.
>>>
>>> On Tue, Jun 12, 2018 at 9:36 PM, Victoria Henry <vhenry(at)pivotal(dot)io>
>>> 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/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 == '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 same
>>> 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
>>>> ​
>>>> Victoria & Joao
>>>>
>>>> Thanks,
>>> Khushboo
>>>
>>>> On Tue, Jun 12, 2018 at 3:10 AM Khushboo Vashi <
>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> Please find the attached updated patch.
>>>>>
>>>>> On Fri, Jun 8, 2018 at 2:21 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>
>>>>>> Hi
>>>>>>
>>>>>> On Thu, Jun 7, 2018 at 11:27 AM, Khushboo Vashi <
>>>>>> khushboo(dot)vashi(at)enterprisedb(dot)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
>