Re: SQL Code Formatting Patch

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: SQL Code Formatting Patch

Dave Page
[Please use the list!]

> -----Original Message-----
> From: Edward Di Geronimo Jr. [mailto:[hidden email]]
> Sent: 08 June 2006 00:55
> To: Dave Page
> Subject: SQL Code Formatting Patch
>
> Hi Dave,
>
> I got sick of the unreadable SQL displayed as the source of
> views, so  
> I redid the formatting code. Complex views are very nice to
> read now.  
> Below is a sample before and after, using the view that
> prompted me to  
> do this. Attached is the patch.

OK, looks quite nice... But (you knew there was a but right?) the brace
positioning around sub selects seems a little off, eg:

------------------------------------
CREATE OR REPLACE VIEW rep_suppreqs AS
 
 SELECT sr_header.sr_guid,
        sr_header.sr_timestamp,
        sr_header.sr_owner,
        sr_header.sr_user,
        sr_header.sr_email,
        sr_header.sr_phone,
        sr_header.sr_fax,
        sr_header.sr_hardware_id,
        sr_header.sr_software_id,
        sr_header.sr_status,
        sr_header.sr_type,
        sr_header.sr_helpdesk,
        sr_header.sr_helpdesk_id,
        sr_header.sr_helpdesk_timestamp,
        sr_header.sr_description,
        sr_header.sr_details,
        sr_header.sr_priority,
        sr_header.sr_updated,
        sr_header.sr_owner_status,
                 
        CASE            
            WHEN sr_header.sr_status::text = 'U'::character
varying::text THEN 'With User'::text            
            WHEN sr_header.sr_status::text = 'O'::character
varying::text THEN 'Open'::text            
            WHEN sr_header.sr_status::text = 'H'::character
varying::text THEN 'With Helpdesk'::text            
            WHEN sr_header.sr_status::text = 'C'::character
varying::text THEN 'Closed'::text            
            ELSE '** UNKNOWN **'::text        
        END  AS status,
                 
        CASE            
            WHEN sr_header.sr_type::text = 'H'::character varying::text
THEN 'Hardware Fault'::text            
            WHEN sr_header.sr_type::text = 'S'::character varying::text
THEN 'Software Fault'::text            
            WHEN sr_header.sr_type::text = 'I'::character varying::text
THEN 'Information Request'::text            
            WHEN sr_header.sr_type::text = 'W'::character varying::text
THEN 'Work Request'::text            
            WHEN sr_header.sr_type::text = 'B'::character varying::text
THEN 'Beta Test Report'::text            
            ELSE '** UNKNOWN **'::text        
        END  AS "type",
        (
              SELECT sys_user.fullname            
                FROM sys_user          
               WHERE sys_user.username::text = sr_header.sr_owner::text)
AS "owner",
        (
              SELECT (((a1.gbl_name::text || ' - '::character
varying::text) || s1.sw_name::text) || ' v'::character varying::text) ||
s1.sw_version::text            
                FROM sw_software s1,
                     gbl_addr_book a1          
               WHERE s1.sw_vendor = a1.gbl_guid
                     AND s1.sw_guid = sr_header.sr_software_id) AS
software,
        (
              SELECT ((((h2.hd_hardware_id::text || ' ('::character
varying::text) || a2.gbl_name::text) || ',
                          '::character varying::text) ||
h2.hd_model::text) || ')'::character varying::text            
                FROM hd_hardware h2,
                     gbl_addr_book a2          
               WHERE h2.hd_manufacturer = a2.gbl_guid
                     AND h2.hd_guid = sr_header.sr_hardware_id) AS
hardware,
        (
              SELECT a3.gbl_name            
                FROM gbl_addr_book a3          
               WHERE a3.gbl_guid = sr_header.sr_helpdesk) AS helpdesk

   FROM sr_header;
------------------------------------

I would expect the subselects to look more like:

        (
              SELECT ((((h2.hd_hardware_id::text || ' ('::character
varying::text) || a2.gbl_name::text) || ',
                          '::character varying::text) ||
h2.hd_model::text) || ')'::character varying::text            
                FROM hd_hardware h2,
                     gbl_addr_book a2          
               WHERE h2.hd_manufacturer = a2.gbl_guid
                     AND h2.hd_guid = sr_header.sr_hardware_id
         ) AS hardware,

Having the braces in non-matched positions decreases the readability I
think. Thoughts?

> Hope you had a nice trip to Paris.

Yes, very good thanks :-)

Regards. Dave

---------------------------(end of broadcast)---------------------------
TIP 2: Don't 'kill -9' the postmaster
Reply | Threaded
Open this post in threaded view
|

Re: SQL Code Formatting Patch

Edward Di Geronimo Jr.
Quoting Dave Page <[hidden email]>:

> I would expect the subselects to look more like:
>
>         (
>               SELECT ((((h2.hd_hardware_id::text || ' ('::character
> varying::text) || a2.gbl_name::text) || ',
>                           '::character varying::text) ||
> h2.hd_model::text) || ')'::character varying::text
>                 FROM hd_hardware h2,
>                      gbl_addr_book a2
>                WHERE h2.hd_manufacturer = a2.gbl_guid
>                      AND h2.hd_guid = sr_header.sr_hardware_id
>          ) AS hardware,
>
> Having the braces in non-matched positions decreases the readability I
> think. Thoughts?

I agree that it would be better as you wrote, and that was my original  
intention, but I'm having trouble pulling that off without side  
effects that make things look much worse. I've got a version that  
handles the parens around the select ok, but, has issues with the  
other parens in that example. The above would come out something like  
this:

         (
               SELECT ((((h2.hd_hardware_id::text || ' ('::character  
varying::text
              ) || a2.gbl_name::text
              ) || ',                          '::character varying::text
              ) || h2.hd_model::text
              ) || ')'::character varying::text
                 FROM hd_hardware h2,
                      gbl_addr_book a2
                WHERE h2.hd_manufacturer = a2.gbl_guid
                      AND h2.hd_guid = sr_header.sr_hardware_id
          ) AS hardware,

I'll look at it a little more, but I don't have high hopes of getting  
it better without making the scanner try to understand the SQL instead  
of the current approach of just reacting to things of interest. I do  
have an idea that may work, but I'm not sure yet. It'll take a while  
for me to work out, and I'm not sure how quickly I'll be able to get  
to it, so you may want to go ahead with the current patch for now.

BTW, there's a table of keywords in the code that defines what should  
be done when they're detected. There's a bool at the end that  
determines if a new line should be inserted before the keyword. You  
may want to change that to false for the CASE keyword. I wasn't sure  
if there were instances where that would backfire, so I figured extra  
newlines were better than not enough of them. My main concern is if  
you try using case somewhere in the where clause.

Ed

----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.



---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

               http://archives.postgresql.org
Reply | Threaded
Open this post in threaded view
|

Re: SQL Code Formatting Patch

Dave Page
In reply to this post by Dave Page
 

> -----Original Message-----
> From: Edward Di Geronimo Jr. [mailto:[hidden email]]
> Sent: 13 June 2006 17:06
> To: Dave Page
> Cc: pgadmin-hackers
> Subject: RE: SQL Code Formatting Patch
>
> I agree that it would be better as you wrote, and that was my
> original  
> intention, but I'm having trouble pulling that off without side  
> effects that make things look much worse. I've got a version that  
> handles the parens around the select ok, but, has issues with the  
> other parens in that example. The above would come out
> something like  
> this:
>
>          (
>                SELECT ((((h2.hd_hardware_id::text || ' ('::character  
> varying::text
>               ) || a2.gbl_name::text
>               ) || ',                          '::character
> varying::text
>               ) || h2.hd_model::text
>               ) || ')'::character varying::text
>                  FROM hd_hardware h2,
>                       gbl_addr_book a2
>                 WHERE h2.hd_manufacturer = a2.gbl_guid
>                       AND h2.hd_guid = sr_header.sr_hardware_id
>           ) AS hardware,

Yeuch.

> I'll look at it a little more, but I don't have high hopes of
> getting  
> it better without making the scanner try to understand the
> SQL instead  
> of the current approach of just reacting to things of interest. I do  
> have an idea that may work, but I'm not sure yet. It'll take a while  
> for me to work out, and I'm not sure how quickly I'll be able to get  
> to it, so you may want to go ahead with the current patch for now.

I'll hold off for now - SQL formatting is probably the second most
contentious issue in pgAdmin, so we're probably best waiting until
everyone's happy.

> BTW, there's a table of keywords in the code that defines
> what should  
> be done when they're detected. There's a bool at the end that  
> determines if a new line should be inserted before the keyword. You  
> may want to change that to false for the CASE keyword. I wasn't sure  
> if there were instances where that would backfire, so I
> figured extra  
> newlines were better than not enough of them. My main concern is if  
> you try using case somewhere in the where clause.

I generally like my CASE's to be seperated, so let's keep it as is for
now.

Cheers, Dave.

---------------------------(end of broadcast)---------------------------
TIP 5: don't forget to increase your free space map settings