On Sun, Mar 27, 2022 at 11:53:58AM -0400, Tom Lane wrote:
> Julien Rouhaud <rjuju123@gmail.com> writes:
> > [ v4-0001-Add-a-pg_get_query_def-wrapper-around-get_query_d.patch ]
>
> This seems about ready to go to me, except for
>
> (1) as an exported API, pg_get_querydef needs a full API spec in its
> header comment. "Read the code to figure out what to do" is not OK
> in my book.
Fixed.
> (2) I don't think this has been thought out too well:
>
> > I also kept the wrapColument and startIdent as they
> > can be easily used by callers.
>
> The appropriate values for these are determined by macros that are
> local in ruleutils.c, so it's not that "easy" for outside callers
> to conform to standard practice. I suppose we could move
> WRAP_COLUMN_DEFAULT etc into ruleutils.h, but is there actually a
> use-case for messing with those?
As far as I can see the wrapColumn and startIndent are independant of the
pretty flags, and don't really have magic numbers for those
(WRAP_COLUMN_DEFAULT sure exists, but the value isn't really surprising).
> I don't see any other exported
> functions that go beyond offering a "bool pretty" option, so
> I think it might be a mistake for this one to be different.
There's the SQL function pg_get_viewdef_wrap() that accept a custom wrapColumn.
That being said I'm totally ok with just exposing a "pretty" parameter and use
WRAP_COLUMN_DEFAULT. In any case I agree that exposing startIndent doesn't
serve any purpose.
I'm attaching a v5 with hopefully a better comment for the function, and only
the "pretty" parameter.
