Thread: Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
Peter Smith <smithpb2250@gmail.com> writes:
> While reviewing a patch for another pg_createsubscriber thread I found
> that multiple TAP tests have a terrible wrapping where the command
> options and their associated oparg are separated on different lines
> instead of paired together nicely. This makes it unnecessarily
> difficult to see what the test is doing.
I think that is mostly the fault of pgperltidy. We did change
the options we use with it awhile back, so maybe now it will honor
your manual changes to its line-wrapping choices. But I wouldn't
bet on that. Did you check what happens if you run the modified
code through pgperltidy?
(If the answer is bad, we could look into making further changes so
that pgperltidy won't override these decisions. But there's no point
in manually patching this if it'll just get undone.)
regards, tom lane
Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
From
Peter Smith
Date:
On Thu, Dec 12, 2024 at 12:41 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Peter Smith <smithpb2250@gmail.com> writes: > > While reviewing a patch for another pg_createsubscriber thread I found > > that multiple TAP tests have a terrible wrapping where the command > > options and their associated oparg are separated on different lines > > instead of paired together nicely. This makes it unnecessarily > > difficult to see what the test is doing. > > I think that is mostly the fault of pgperltidy. We did change > the options we use with it awhile back, so maybe now it will honor > your manual changes to its line-wrapping choices. But I wouldn't > bet on that. Did you check what happens if you run the modified > code through pgperltidy? > > (If the answer is bad, we could look into making further changes so > that pgperltidy won't override these decisions. But there's no point > in manually patching this if it'll just get undone.) > Thanks for your suggestion. As you probably suspected, the answer is bad. I ran pgperltidy on the "fixed" file: [postgres@CentOS7-x64 oss_postgres_misc]$ src/tools/pgindent/pgperltidy src/bin/pg_basebackup/t/040_pg_createsubscriber.pl This reverted it to how it currently looks on master. The strange thing is there are other commands in that file very similar to the ones I had changed but those already looked good, yet they remained unaffected by the pgperltidy. Why? ====== Kind Regards, Peter Smith. Fujitsu Australia
Peter Smith <smithpb2250@gmail.com> writes:
> The strange thing is there are other commands in that file very
> similar to the ones I had changed but those already looked good, yet
> they remained unaffected by the pgperltidy. Why?
You sure it's not just luck-of-the-draw? I think that perltidy
is just splitting the lines based on length, so sometimes related
options would be kept together and sometimes not.
regards, tom lane
Re: pg_createsubscriber TAP test wrapping makes command options hard to read.
From
Peter Smith
Date:
On Thu, Dec 12, 2024 at 1:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Peter Smith <smithpb2250@gmail.com> writes: > > The strange thing is there are other commands in that file very > > similar to the ones I had changed but those already looked good, yet > > they remained unaffected by the pgperltidy. Why? > > You sure it's not just luck-of-the-draw? I think that perltidy > is just splitting the lines based on length, so sometimes related > options would be kept together and sometimes not. > TBH, I have no idea what logic perltidy uses. I did find some configurations here [1] (are those what it pgperltidy uses?) but those claim max line length is 78 which I didn;t come anywhere near exceeding. After some more experimentation, I've noticed that it is trying to keep only 2 items on each line. So whether it looks good or not seems to depend if there is an even or odd number of options without arguments up-front. Maybe those perltidy "tightness" switches? So, AFAICT I can workaround the perltidy wrapping just by putting all the noarg options at the bottom of the command, then all the option/optarg pairs (ie 2s) will stay together. I can post another patch to do it this way unless you think it is too hacky. ====== [1] https://github.com/postgres/postgres/blob/master/src/tools/pgindent/perltidyrc Kind Regards, Peter Smith. Fujitsu Australia