public inbox for [email protected]  

help / color / mirror / Atom feed

From: Murtuza Zabuawala <[email protected]>
To: Neel Patel <[email protected]>
To: Dave Page <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: PATCH: added "Collation" & "Catalog Objects" nodes in pgAdmin4
Date: Tue, 19 Jan 2016 10:31:02 +0530
Message-ID: <[email protected]> (raw)
In-Reply-To: <CACCA4P2JhEmF8LqmwEaZP-P9HQ1D0rnWU1V3aQvpfawxwbT0vw@mail.gmail.com>
References: <[email protected]>
	<CA+OCxozBsx0v0Be_bkSi7EP3BxwMJ9kn3vGgae1N4oVVjp0+6w@mail.gmail.com>
	<CACCA4P2JhEmF8LqmwEaZP-P9HQ1D0rnWU1V3aQvpfawxwbT0vw@mail.gmail.com>
List-Unsubscribe:  <mailto:[email protected]?body=unsub%20pgadmin-hackers>

Thanks Neel.

I'll work on below mentioned issues & send new patch.

Regards,
Murtuza

On Monday 18 January 2016 10:12 PM, Neel Patel wrote:
> Hi Murtuza,
>
> Please find below review comments for the collation node.
>
> 1. "Owner" field should be changed from text control to 
> Backform.NodeListByNameControl.
>
> 2. Remove "Use Slony" option, we will implement it as separate module.
>
> 3. Use 2 space indentation instead of 4 space in javascript file.
>
> 4. In some of the sql file, 'qtIdent' and 'qtLiteral' is not used. 
> Please check all the SQL files and make sure we use the same.
>     e.g. In update.sql  -- Change  '{{ data.description }}'   to {{ 
> data.description|qtLiteral }}
>
> 5. While creating the new collation, there is a spelling  mistake in 
> "Definition" tab text. Currently it is displayed as "Defination" which 
> is wrong.
>
> 6. Wrong SQL is getting generated when we create the new collation 
> from "Copy collation" as below. Please correct the generated SQL.
>
>     CREATE COLLATION public.my_collation FROM pg_catalog.\;
>
> 7. If we "Edit" the existing created collation with collation Name 
> then below wrong SQL is getting generated. In below SQL - if user 
> change the name of the collation then "... RENAME TO ..." query should 
> be first executed then other modified parameters should be applied.
> We also noticed that when we change only collation name then also for 
> the unchanged parameters, queries are getting generated which should 
> be corrected.  Query should be generated only for modified parameters.
>
> Below is the wrong SQL.
>
>     ALTER COLLATION my_schema.my_collation_1_up
>     OWNER TO ;
>     COMMENT ON COLLATION my_schema.my_collation_1_up
>     IS 'testing comment....';
>     ALTER COLLATION my_schema.my_collation_1_up
>     RENAME TO my_collation_1;
>     ALTER COLLATION my_schema.my_collation_1 SET SCHEMA None;
>
> 8.  When we create the new collation with below parameters then we are 
> getting "*IndexError: list index out of range*" error at python side.
>      Use below parameter to reproduce the error.
>
>      CREATE COLLATION public.collation_23
>      (
> LOCALE = 'en_AG.utf8'
>      );
>     ALTER COLLATION public.collation_23
>     OWNER TO postgres;
>     COMMENT ON COLLATION public.collation_23
>     IS 'My comment....';
> 9. As per Dave's comment, change the copyright year to 2016 and some 
> of the spelling mistakes.
>
> 10. In "validate" function in collation.js, multiple time 
> "this.get('<column_name>')" is used. Instead of using  multiple time, 
> we can assign in one variable
>       and use that variable in all the places.
>
> Do let us know for any comments/issues.
>
> Thanks,
> Neel Patel
>
> On Wed, Jan 6, 2016 at 6:10 PM, Dave Page <[email protected] 
> <mailto:[email protected]>> wrote:
>
>     On Wed, Jan 6, 2016 at 12:18 PM, Murtuza Zabuawala
>     <[email protected]
>     <mailto:[email protected]>> wrote:
>     > Hi,
>     >
>     > Please find patch to add "Collation" & "Catalog Objects" nodes
>     in pgAdmin4.
>     >
>     > Please note that this patch is mainly for "Collation" & "Catalog
>     Objects"
>     > nodes, Schema/Catalog node is not yet complete as we are yet to
>     implement
>     > privileges control for the same.
>
>     Thanks - a couple of quick comments:
>
>     - Please ensure the copyright notices are updated for 2016.
>
>     - The text:
>
>     Below SQL will ....
>
>     Should be:
>
>     The SQL below will ....
>
>     --
>     Dave Page
>     Blog: http://pgsnake.blogspot.com
>     Twitter: @pgsnake
>
>     EnterpriseDB UK: http://www.enterprisedb.com
>     The Enterprise PostgreSQL Company
>
>
>     --
>     Sent via pgadmin-hackers mailing list
>     ([email protected]
>     <mailto:[email protected]>)
>     To make changes to your subscription:
>     http://www.postgresql.org/mailpref/pgadmin-hackers
>
>

view thread (5+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: PATCH: added "Collation" & "Catalog Objects" nodes in pgAdmin4
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox