Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, andadd support procedure for PG v11 - Mailing list pgadmin-hackers
From | Victoria Henry |
---|---|
Subject | Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, andadd support procedure for PG v11 |
Date | |
Msg-id | CANxYE3+kOqNGQEEOp6SQ+L3Ua-1bnq_KPVAA=0RvUBHiGnEseA@mail.gmail.com Whole thread Raw |
In response to | Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, andadd support procedure for PG v11 (Dave Page <dpage@pgadmin.org>) |
Responses |
Re: [pgAdmin4][Patch]: RM 3362 - Fix the functions for PG v11, andadd support procedure for PG v11
|
List | pgadmin-hackers |
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 codeThis 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@pgadmin.org> 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 <vhenry@pivotal.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 otherstrigger_funcs_utils
.Fixed.Thanks
Victoria & JoaoThanks,KhushbooOn 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:HiOn 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 modesFixed- 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$BEGINraise 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
pgadmin-hackers by date: