Thread: Re: SQL Code Formatting Patch

[Please use the list!]

> -----Original Message-----
> From: Edward Di Geronimo Jr. [mailto:edigeronimo@xtracards.com]
> 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

Quoting Dave Page <dpage@vale-housing.co.uk>:

> 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.