Thread: perlcritic and perltidy
The attached patch allows a clean run from the following script adapted
from pgperltidy:
{
find . -type f -a \( -name '*.pl' -o -name '*.pm' \) -print
find . -type f -perm -100 -exec file {} \; -print \
| egrep -i ':.*perl[0-9]*\>' \
| cut -d: -f1
} \
| sort -u | xargs perlcritic --exclude ProhibitLeadingZeros
The changes are
* disable perlcritic on Gen_dummy_probes.pl, since it's generated code
from s2p
* protect a couple of package declarations in plperl code from
perltidy since it splits the lines and renders the 'no critic'
directives there useless
* mark a string eval in Catalog.pm with 'no critic', since it's
clearly necessary.
We should probably set up a policy file for perlcritic that turns off or
at least lowers the severity of the ProhibitLeadingZeros policy. Making
it severity 5 seems a bit odd.
w.r.t. perltidy, I note that our policy has these two lines:
--vertical-tightness=2
--vertical-tightness-closing=2
I've been looking at syncing the buildfarm client with our core code
perltidy settings. However, I don't actually like these two and I've
decided to exercise some editorial discretion and not use them.
Note that the perltidy man page does suggest that these can make things
less readable, and it also states unequivocally "You must also use the
-lp flag when you use the -vt flag". That is the --line-up-parentheses
flag and it's something we don't use. Enabling it would generate about
12k lines of diff.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> The attached patch allows a clean run from the following script adapted
> from pgperltidy:
I'm hardly a perl expert, but those changes look reasonable.
> w.r.t. perltidy, I note that our policy has these two lines:
> --vertical-tightness=2
> --vertical-tightness-closing=2
> I've been looking at syncing the buildfarm client with our core code
> perltidy settings. However, I don't actually like these two and I've
> decided to exercise some editorial discretion and not use them.
Okay ...
> Note that the perltidy man page does suggest that these can make things
> less readable, and it also states unequivocally "You must also use the
> -lp flag when you use the -vt flag". That is the --line-up-parentheses
> flag and it's something we don't use. Enabling it would generate about
> 12k lines of diff.
What sort of changes do we get if we remove those two flags as you prefer?
It'd help to see some examples.
Since we just went to a new perltidy version, and made some other
policy changes for it, in HEAD, it'd make sense to make any further
changes in this same release cycle rather than drip drip drip over
multiple cycles. We just need to get some consensus about what
style we like.
regards, tom lane
On 05/06/2018 11:53 AM, Tom Lane wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
>> The attached patch allows a clean run from the following script adapted
>> from pgperltidy:
> I'm hardly a perl expert, but those changes look reasonable.
>
>> w.r.t. perltidy, I note that our policy has these two lines:
>> --vertical-tightness=2
>> --vertical-tightness-closing=2
>> I've been looking at syncing the buildfarm client with our core code
>> perltidy settings. However, I don't actually like these two and I've
>> decided to exercise some editorial discretion and not use them.
> Okay ...
>
>> Note that the perltidy man page does suggest that these can make things
>> less readable, and it also states unequivocally "You must also use the
>> -lp flag when you use the -vt flag". That is the --line-up-parentheses
>> flag and it's something we don't use. Enabling it would generate about
>> 12k lines of diff.
> What sort of changes do we get if we remove those two flags as you prefer?
> It'd help to see some examples.
>
> Since we just went to a new perltidy version, and made some other
> policy changes for it, in HEAD, it'd make sense to make any further
> changes in this same release cycle rather than drip drip drip over
> multiple cycles. We just need to get some consensus about what
> style we like.
Essentially it adds some vertical whitespace to structures so that the
enclosing braces etc appear on their own lines. A very typical change
looks like this:
- { code => $code,
+ {
+ code => $code,
ucs => $ucs,
comment => $rest,
direction => $direction,
f => $in_file,
- l => $. };
+ l => $.
+ };
I am attaching the diff for a complete run with these two settings
removed. It's about 10k lines.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 05/06/2018 11:53 AM, Tom Lane wrote:
>> What sort of changes do we get if we remove those two flags as you prefer?
>> It'd help to see some examples.
> Essentially it adds some vertical whitespace to structures so that the
> enclosing braces etc appear on their own lines. A very typical change
> looks like this:
> - { code => $code,
> + {
> + code => $code,
> ucs => $ucs,
> comment => $rest,
> direction => $direction,
> f => $in_file,
> - l => $. };
> + l => $.
> + };
Hm. I have no strong opinion about whether this looks better or not;
people who write more Perl than I do ought to weigh in.
However, I do want to note that we've chosen the shorter style for
the catalog .dat files, and that's enforced by reformat_dat_file.pl.
I'd be against changing that decision, because one of the goals for
the .dat file format was to minimize the risk of patches applying in
the wrong place. Near-content-free lines containing just "{" or "},"
would increase that risk by reducing the uniqueness of patch context
lines.
It's not essential for the .dat files to match the Perl layout we use
elsewhere, of course. But perhaps consistency is one factor to
consider here.
regards, tom lane
Greetings,
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> > On 05/06/2018 11:53 AM, Tom Lane wrote:
> >> What sort of changes do we get if we remove those two flags as you prefer?
> >> It'd help to see some examples.
>
> > Essentially it adds some vertical whitespace to structures so that the
> > enclosing braces etc appear on their own lines. A very typical change
> > looks like this:
>
> > - { code => $code,
> > + {
> > + code => $code,
> > ucs => $ucs,
> > comment => $rest,
> > direction => $direction,
> > f => $in_file,
> > - l => $. };
> > + l => $.
> > + };
>
> Hm. I have no strong opinion about whether this looks better or not;
> people who write more Perl than I do ought to weigh in.
I definitely prefer to have the braces on their own line- makes working
with the files a lot easier when you've got a lot of hashes
(particularly thinking about the hashes for the pg_dump regression
tests..). Having them on independent lines would have saved me quite a
few keystrokes when I reworked those tests.
> However, I do want to note that we've chosen the shorter style for
> the catalog .dat files, and that's enforced by reformat_dat_file.pl.
> I'd be against changing that decision, because one of the goals for
> the .dat file format was to minimize the risk of patches applying in
> the wrong place. Near-content-free lines containing just "{" or "},"
> would increase that risk by reducing the uniqueness of patch context
> lines.
I can understand that concern, though I don't think it really applies as
much to other the other perl code.
Thanks!
Stephen
Attachment
On Sun, May 06, 2018 at 01:46:28PM -0400, Stephen Frost wrote: > I definitely prefer to have the braces on their own line- makes working > with the files a lot easier when you've got a lot of hashes > (particularly thinking about the hashes for the pg_dump regression > tests..). Having them on independent lines would have saved me quite a > few keystrokes when I reworked those tests. Agreed with Stephen's argument. Let's keep the braces on the same line. I have also been annoyed a couple of times with the format which adds a new line just for a brace. -- Michael
Attachment
Michael, * Michael Paquier (michael@paquier.xyz) wrote: > On Sun, May 06, 2018 at 01:46:28PM -0400, Stephen Frost wrote: > > I definitely prefer to have the braces on their own line- makes working > > with the files a lot easier when you've got a lot of hashes > > (particularly thinking about the hashes for the pg_dump regression > > tests..). Having them on independent lines would have saved me quite a > > few keystrokes when I reworked those tests. > > Agreed with Stephen's argument. Let's keep the braces on the same > line. I have also been annoyed a couple of times with the format which > adds a new line just for a brace. While I appreciate the support, I'm not sure that you're actually agreeing with me.. I was arguing that braces should be on their own line and therefore there would be a new line for the brace. Specifically, when moving lines between hashes, it's annoying to have to also worry about if the line being copied/moved has braces at the end or not- much easier if they don't and the braces are on their own line. Thanks! Stephen
Attachment
On Sun, May 06, 2018 at 09:14:06PM -0400, Stephen Frost wrote: > While I appreciate the support, I'm not sure that you're actually > agreeing with me.. I was arguing that braces should be on their own > line and therefore there would be a new line for the brace. > Specifically, when moving lines between hashes, it's annoying to have to > also worry about if the line being copied/moved has braces at the end or > not- much easier if they don't and the braces are on their own line. I should have read that twice. Yes we are not on the same line. Even if a brace is on a different line, per your argument it would still be nicer to add a comma at the end of each last element of a hash or an array, which is what you have done in the tests of pg_dump, but not something that the proposed patch does consistently. If the formatting is automated, the way chosen does not matter much, but the extra last comma should be consistently present as well? -- Michael
Attachment
On 5/6/18 12:13, Andrew Dunstan wrote:
> Essentially it adds some vertical whitespace to structures so that the
> enclosing braces etc appear on their own lines. A very typical change
> looks like this:
>
> - { code => $code,
> + {
> + code => $code,
> ucs => $ucs,
> comment => $rest,
> direction => $direction,
> f => $in_file,
> - l => $. };
> + l => $.
> + };
The proposed changes certainly match the style we use in C better, which
is what some of the other settings were also informed by. So I'm in
favor of the changes -- for braces.
For parentheses, I'm not sure whether this is a good idea:
diff --git a/src/backend/utils/mb/Unicode/UCS_to_EUC_CN.pl
b/src/backend/utils/mb/Unicode/UCS_to_EUC_CN.pl
index 2971e64..0d3184c 100755
--- a/src/backend/utils/mb/Unicode/UCS_to_EUC_CN.pl
+++ b/src/backend/utils/mb/Unicode/UCS_to_EUC_CN.pl
@@ -40,8 +40,11 @@ while (<$in>)
next if (($code & 0xFF) < 0xA1);
next
if (
- !( $code >= 0xA100 && $code <= 0xA9FF
- || $code >= 0xB000 && $code <= 0xF7FF));
+ !(
+ $code >= 0xA100 && $code <= 0xA9FF
+ || $code >= 0xB000 && $code <= 0xF7FF
+ )
+ );
next if ($code >= 0xA2A1 && $code <= 0xA2B0);
next if ($code >= 0xA2E3 && $code <= 0xA2E4);
In a manual C-style indentation, this would just be
next if (!($code >= 0xA100 && $code <= 0xA9FF
|| $code >= 0xB000 && $code <= 0xF7FF));
but somehow the indent runs have managed to spread this compact
expression over the entire screen.
Can we have separate settings for braces and parentheses?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Greetings, * Michael Paquier (michael@paquier.xyz) wrote: > On Sun, May 06, 2018 at 09:14:06PM -0400, Stephen Frost wrote: > > While I appreciate the support, I'm not sure that you're actually > > agreeing with me.. I was arguing that braces should be on their own > > line and therefore there would be a new line for the brace. > > Specifically, when moving lines between hashes, it's annoying to have to > > also worry about if the line being copied/moved has braces at the end or > > not- much easier if they don't and the braces are on their own line. > > I should have read that twice. Yes we are not on the same line. Even > if a brace is on a different line, per your argument it would still be > nicer to add a comma at the end of each last element of a hash or an > array, which is what you have done in the tests of pg_dump, but not > something that the proposed patch does consistently. If the formatting > is automated, the way chosen does not matter much, but the extra last > comma should be consistently present as well? Yes, that would be nice as well, as you'd be able to move entries around more easily that way. Thanks! Stephen
Attachment
On 5/8/18 8:11 AM, Stephen Frost wrote: > Greetings, > > * Michael Paquier (michael@paquier.xyz) wrote: >> On Sun, May 06, 2018 at 09:14:06PM -0400, Stephen Frost wrote: >>> While I appreciate the support, I'm not sure that you're actually >>> agreeing with me.. I was arguing that braces should be on their own >>> line and therefore there would be a new line for the brace. >>> Specifically, when moving lines between hashes, it's annoying to have to >>> also worry about if the line being copied/moved has braces at the end or >>> not- much easier if they don't and the braces are on their own line. >> >> I should have read that twice. Yes we are not on the same line. Even >> if a brace is on a different line, per your argument it would still be >> nicer to add a comma at the end of each last element of a hash or an >> array, which is what you have done in the tests of pg_dump, but not >> something that the proposed patch does consistently. If the formatting >> is automated, the way chosen does not matter much, but the extra last >> comma should be consistently present as well? > > Yes, that would be nice as well, as you'd be able to move entries around > more easily that way. I'm a fan of the final comma as it makes diffs less noisy. Regards, -- -David david@pgmasters.net
Attachment
On 05/08/2018 08:31 AM, David Steele wrote:
> On 5/8/18 8:11 AM, Stephen Frost wrote:
>> Greetings,
>>
>> * Michael Paquier (michael@paquier.xyz) wrote:
>>> On Sun, May 06, 2018 at 09:14:06PM -0400, Stephen Frost wrote:
>>>> While I appreciate the support, I'm not sure that you're actually
>>>> agreeing with me.. I was arguing that braces should be on their own
>>>> line and therefore there would be a new line for the brace.
>>>> Specifically, when moving lines between hashes, it's annoying to have to
>>>> also worry about if the line being copied/moved has braces at the end or
>>>> not- much easier if they don't and the braces are on their own line.
>>> I should have read that twice. Yes we are not on the same line. Even
>>> if a brace is on a different line, per your argument it would still be
>>> nicer to add a comma at the end of each last element of a hash or an
>>> array, which is what you have done in the tests of pg_dump, but not
>>> something that the proposed patch does consistently. If the formatting
>>> is automated, the way chosen does not matter much, but the extra last
>>> comma should be consistently present as well?
>> Yes, that would be nice as well, as you'd be able to move entries around
>> more easily that way.
> I'm a fan of the final comma as it makes diffs less noisy.
>
Me too.
AFAICT there is no perltidy setting to add them where they are missing.
There is a perlcritic setting to detect them in lists, however. Here is
the output:
andrew@emma:pg_head (master)$ { find . -type f -a \( -name
'*.pl' -o -name '*.pm' \) -print; find . -type f -perm -100
-exec file {} \; -print | egrep -i
':.*perl[0-9]*\>' | cut -d: -f1; } | sort -u |
xargs perlcritic --quiet --single CodeLayout::RequireTrailingCommas
./src/backend/catalog/Catalog.pm: List declaration without trailing
comma at line 30, column 23. See page 17 of PBP. (Severity: 1)
./src/backend/catalog/genbki.pl: List declaration without trailing comma
at line 242, column 19. See page 17 of PBP. (Severity: 1)
./src/backend/catalog/genbki.pl: List declaration without trailing comma
at line 627, column 20. See page 17 of PBP. (Severity: 1)
./src/backend/utils/mb/Unicode/UCS_to_most.pl: List declaration without
trailing comma at line 23, column 16. See page 17 of PBP. (Severity: 1)
./src/backend/utils/mb/Unicode/UCS_to_SJIS.pl: List declaration without
trailing comma at line 21, column 19. See page 17 of PBP. (Severity: 1)
./src/interfaces/ecpg/preproc/check_rules.pl: List declaration without
trailing comma at line 38, column 20. See page 17 of PBP. (Severity: 1)
./src/test/perl/PostgresNode.pm: List declaration without trailing comma
at line 1468, column 14. See page 17 of PBP. (Severity: 1)
./src/test/perl/PostgresNode.pm: List declaration without trailing comma
at line 1657, column 16. See page 17 of PBP. (Severity: 1)
./src/test/perl/PostgresNode.pm: List declaration without trailing comma
at line 1697, column 12. See page 17 of PBP. (Severity: 1)
./src/test/recovery/t/003_recovery_targets.pl: List declaration without
trailing comma at line 119, column 20. See page 17 of PBP. (Severity: 1)
./src/test/recovery/t/003_recovery_targets.pl: List declaration without
trailing comma at line 125, column 20. See page 17 of PBP. (Severity: 1)
./src/test/recovery/t/003_recovery_targets.pl: List declaration without
trailing comma at line 131, column 20. See page 17 of PBP. (Severity: 1)
./src/tools/msvc/Install.pm: List declaration without trailing comma at
line 22, column 28. See page 17 of PBP. (Severity: 1)
./src/tools/msvc/Install.pm: List declaration without trailing comma at
line 81, column 17. See page 17 of PBP. (Severity: 1)
./src/tools/msvc/Install.pm: List declaration without trailing comma at
line 653, column 14. See page 17 of PBP. (Severity: 1)
./src/tools/msvc/Install.pm: List declaration without trailing comma at
line 709, column 15. See page 17 of PBP. (Severity: 1)
./src/tools/msvc/Mkvcbuild.pm: List declaration without trailing comma
at line 43, column 24. See page 17 of PBP. (Severity: 1)
./src/tools/msvc/Mkvcbuild.pm: List declaration without trailing comma
at line 55, column 29. See page 17 of PBP. (Severity: 1)
./src/tools/msvc/Mkvcbuild.pm: List declaration without trailing comma
at line 59, column 31. See page 17 of PBP. (Severity: 1)
./src/tools/msvc/Mkvcbuild.pm: List declaration without trailing comma
at line 75, column 25. See page 17 of PBP. (Severity: 1)
./src/tools/msvc/Mkvcbuild.pm: List declaration without trailing comma
at line 623, column 15. See page 17 of PBP. (Severity: 1)
./src/tools/msvc/vcregress.pl: List declaration without trailing comma
at line 102, column 13. See page 17 of PBP. (Severity: 1)
./src/tools/msvc/vcregress.pl: List declaration without trailing comma
at line 121, column 13. See page 17 of PBP. (Severity: 1)
./src/tools/msvc/vcregress.pl: List declaration without trailing comma
at line 147, column 17. See page 17 of PBP. (Severity: 1)
./src/tools/msvc/vcregress.pl: List declaration without trailing comma
at line 167, column 13. See page 17 of PBP. (Severity: 1)
./src/tools/msvc/vcregress.pl: List declaration without trailing comma
at line 352, column 14. See page 17 of PBP. (Severity: 1)
./src/tools/msvc/vcregress.pl: List declaration without trailing comma
at line 404, column 13. See page 17 of PBP. (Severity: 1)
./src/tools/msvc/vcregress.pl: List declaration without trailing comma
at line 554, column 10. See page 17 of PBP. (Severity: 1)
I'll take a look those. There doesn't seem to be a reliable one to
detect them in things other than lists (like anonymous hash and array
contructors using {} and []). There is one in the Pulp collection of
policies, but it doesn't seem to work very well.
But this is all a bit away from the original discussion.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 05/08/2018 07:53 AM, Peter Eisentraut wrote:
> On 5/6/18 12:13, Andrew Dunstan wrote:
>> Essentially it adds some vertical whitespace to structures so that the
>> enclosing braces etc appear on their own lines. A very typical change
>> looks like this:
>>
>> - { code => $code,
>> + {
>> + code => $code,
>> ucs => $ucs,
>> comment => $rest,
>> direction => $direction,
>> f => $in_file,
>> - l => $. };
>> + l => $.
>> + };
> The proposed changes certainly match the style we use in C better, which
> is what some of the other settings were also informed by. So I'm in
> favor of the changes -- for braces.
>
> For parentheses, I'm not sure whether this is a good idea:
>
> diff --git a/src/backend/utils/mb/Unicode/UCS_to_EUC_CN.pl
> b/src/backend/utils/mb/Unicode/UCS_to_EUC_CN.pl
> index 2971e64..0d3184c 100755
> --- a/src/backend/utils/mb/Unicode/UCS_to_EUC_CN.pl
> +++ b/src/backend/utils/mb/Unicode/UCS_to_EUC_CN.pl
> @@ -40,8 +40,11 @@ while (<$in>)
> next if (($code & 0xFF) < 0xA1);
> next
> if (
> - !( $code >= 0xA100 && $code <= 0xA9FF
> - || $code >= 0xB000 && $code <= 0xF7FF));
> + !(
> + $code >= 0xA100 && $code <= 0xA9FF
> + || $code >= 0xB000 && $code <= 0xF7FF
> + )
> + );
>
> next if ($code >= 0xA2A1 && $code <= 0xA2B0);
> next if ($code >= 0xA2E3 && $code <= 0xA2E4);
>
> In a manual C-style indentation, this would just be
>
> next if (!($code >= 0xA100 && $code <= 0xA9FF
> || $code >= 0xB000 && $code <= 0xF7FF));
>
> but somehow the indent runs have managed to spread this compact
> expression over the entire screen.
>
> Can we have separate settings for braces and parentheses?
>
Yes. there are separate settings for the three types of brackets. Here's
what happens if we restrict the vertical tightness settings to parentheses.
I think that's an unambiguous improvement.
Despite what the perltidy manual page says about needing to use
--line-up-parentheses with the vertical-tightness, I don't think we
should use it, I find the results fairly ugly. Also, I note that
according to the docs the -pbp setting includes -vt=2 without including
-lp, so they don't seem terribly consistent here.
So in summary let's just go with
--paren-vertical-tightness=2
--paren-vertical-tightness-closing=2
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Andrew Dunstan wrote: > Yes. there are separate settings for the three types of brackets. Here's > what happens if we restrict the vertical tightness settings to parentheses. > > I think that's an unambiguous improvement. LGTM. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 5/8/18 11:39, Alvaro Herrera wrote: > Andrew Dunstan wrote: > >> Yes. there are separate settings for the three types of brackets. Here's >> what happens if we restrict the vertical tightness settings to parentheses. >> >> I think that's an unambiguous improvement. > > LGTM. Yes, that looks better. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 05/08/2018 10:06 AM, Andrew Dunstan wrote:
> { find . -type f -a \( -name
> '*.pl' -o -name '*.pm' \) -print; find . -type f -perm -100
> -exec file {} \; -print | egrep -i
> ':.*perl[0-9]*\>' | cut -d: -f1; } | sort -u |
> xargs perlcritic --quiet --single CodeLayout::RequireTrailingCommas
Here's a diff of all the places it found fixed. At this stage I don't
think it's worth it. If someone wants to write a perlcritic policy that
identifies missing trailing commas reasonably comprehensively, we can
look again. Otherwise we should just clean them up as we come across them.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Greetings,
* Andrew Dunstan (andrew.dunstan@2ndquadrant.com) wrote:
> On 05/08/2018 10:06 AM, Andrew Dunstan wrote:
> > { find . -type f -a \( -name
> > '*.pl' -o -name '*.pm' \) -print; find . -type f -perm -100
> > -exec file {} \; -print | egrep -i
> > ':.*perl[0-9]*\>' | cut -d: -f1; } | sort -u |
> > xargs perlcritic --quiet --single CodeLayout::RequireTrailingCommas
>
> Here's a diff of all the places it found fixed. At this stage I don't
> think it's worth it. If someone wants to write a perlcritic policy that
> identifies missing trailing commas reasonably comprehensively, we can
> look again. Otherwise we should just clean them up as we come across them.
[...]
> diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
> index f387c86..ac19682 100644
> --- a/src/backend/catalog/Catalog.pm
> +++ b/src/backend/catalog/Catalog.pm
> @@ -34,7 +34,7 @@ sub ParseHeader
> 'Oid' => 'oid',
> 'NameData' => 'name',
> 'TransactionId' => 'xid',
> - 'XLogRecPtr' => 'pg_lsn');
> + 'XLogRecPtr' => 'pg_lsn',);
>
> my %catalog;
> my $declaring_attributes = 0;
There's not much point adding the ',' unless you're also putting the
');' on the next line, is there..?
Or is that going to be handled in a follow-up patch?
Thanks!
Stephen
Attachment
Stephen Frost wrote: > * Andrew Dunstan (andrew.dunstan@2ndquadrant.com) wrote: > > > - 'XLogRecPtr' => 'pg_lsn'); > > + 'XLogRecPtr' => 'pg_lsn',); > > There's not much point adding the ',' unless you're also putting the > ');' on the next line, is there..? > > Or is that going to be handled in a follow-up patch? IMO we should classify this one as pointless uglification and move on. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Greetings, * Alvaro Herrera (alvherre@2ndquadrant.com) wrote: > Stephen Frost wrote: > > > * Andrew Dunstan (andrew.dunstan@2ndquadrant.com) wrote: > > > > > - 'XLogRecPtr' => 'pg_lsn'); > > > + 'XLogRecPtr' => 'pg_lsn',); > > > > There's not much point adding the ',' unless you're also putting the > > ');' on the next line, is there..? > > > > Or is that going to be handled in a follow-up patch? > > IMO we should classify this one as pointless uglification and move on. I'm all for the change if we actually get to a result where the lines can be moved and you don't have to go muck with the extra stuff at the end of the line (add a comma, or remove a comma, remove or add the parens, etc). If we aren't going all the way to get to that point then I tend to agree that it's not a useful change to make. Thanks! Stephen
Attachment
On 05/08/2018 12:51 PM, Stephen Frost wrote:
> Greetings,
>
> * Andrew Dunstan (andrew.dunstan@2ndquadrant.com) wrote:
>> On 05/08/2018 10:06 AM, Andrew Dunstan wrote:
>>> { find . -type f -a \( -name
>>> '*.pl' -o -name '*.pm' \) -print; find . -type f -perm -100
>>> -exec file {} \; -print | egrep -i
>>> ':.*perl[0-9]*\>' | cut -d: -f1; } | sort -u |
>>> xargs perlcritic --quiet --single CodeLayout::RequireTrailingCommas
>> Here's a diff of all the places it found fixed. At this stage I don't
>> think it's worth it. If someone wants to write a perlcritic policy that
>> identifies missing trailing commas reasonably comprehensively, we can
>> look again. Otherwise we should just clean them up as we come across them.
> [...]
>> diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
>> index f387c86..ac19682 100644
>> --- a/src/backend/catalog/Catalog.pm
>> +++ b/src/backend/catalog/Catalog.pm
>> @@ -34,7 +34,7 @@ sub ParseHeader
>> 'Oid' => 'oid',
>> 'NameData' => 'name',
>> 'TransactionId' => 'xid',
>> - 'XLogRecPtr' => 'pg_lsn');
>> + 'XLogRecPtr' => 'pg_lsn',);
>>
>> my %catalog;
>> my $declaring_attributes = 0;
> There's not much point adding the ',' unless you're also putting the
> ');' on the next line, is there..?
No, not really.
>
> Or is that going to be handled in a follow-up patch?
>
No, the current proposal is to keep the vertical tightness settings for
parentheses, which is precisely this set of cases, because otherwise
there are some ugly code efects (see Peter's email upthread)
So I think we're all in agreement to fortget this trailing comma thing.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew, * Andrew Dunstan (andrew.dunstan@2ndquadrant.com) wrote: > On 05/08/2018 12:51 PM, Stephen Frost wrote: > > * Andrew Dunstan (andrew.dunstan@2ndquadrant.com) wrote: > > There's not much point adding the ',' unless you're also putting the > > ');' on the next line, is there..? > > No, not really. > > > Or is that going to be handled in a follow-up patch? > > No, the current proposal is to keep the vertical tightness settings for > parentheses, which is precisely this set of cases, because otherwise > there are some ugly code efects (see Peter's email upthread) > > So I think we're all in agreement to fortget this trailing comma thing. Well, agreed, for parentheses, but for curly-brace blocks, it'd be nice to have them since those will end up on their own line, right? Thanks! Stephen
Attachment
On 05/08/2018 01:18 PM, Stephen Frost wrote: > Andrew, > > * Andrew Dunstan (andrew.dunstan@2ndquadrant.com) wrote: >> On 05/08/2018 12:51 PM, Stephen Frost wrote: >>> * Andrew Dunstan (andrew.dunstan@2ndquadrant.com) wrote: >>> There's not much point adding the ',' unless you're also putting the >>> ');' on the next line, is there..? >> No, not really. >> >>> Or is that going to be handled in a follow-up patch? >> No, the current proposal is to keep the vertical tightness settings for >> parentheses, which is precisely this set of cases, because otherwise >> there are some ugly code efects (see Peter's email upthread) >> >> So I think we're all in agreement to fortget this trailing comma thing. > Well, agreed, for parentheses, but for curly-brace blocks, it'd be nice to > have them since those will end up on their own line, right? > Yes, but there isn't a perlcritic policy I can find that detects them reliably. If you know of one we can revisit it. Specifically, the one from the Pulp collection called RequireTrailingCommaAtNewline didn't work very well when I tried it. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew, * Andrew Dunstan (andrew.dunstan@2ndquadrant.com) wrote: > On 05/08/2018 01:18 PM, Stephen Frost wrote: > > * Andrew Dunstan (andrew.dunstan@2ndquadrant.com) wrote: > >> On 05/08/2018 12:51 PM, Stephen Frost wrote: > >>> * Andrew Dunstan (andrew.dunstan@2ndquadrant.com) wrote: > >>> There's not much point adding the ',' unless you're also putting the > >>> ');' on the next line, is there..? > >> No, not really. > >> > >>> Or is that going to be handled in a follow-up patch? > >> No, the current proposal is to keep the vertical tightness settings for > >> parentheses, which is precisely this set of cases, because otherwise > >> there are some ugly code efects (see Peter's email upthread) > >> > >> So I think we're all in agreement to fortget this trailing comma thing. > > Well, agreed, for parentheses, but for curly-brace blocks, it'd be nice to > > have them since those will end up on their own line, right? > > Yes, but there isn't a perlcritic policy I can find that detects them > reliably. If you know of one we can revisit it. Specifically, the one > from the Pulp collection called RequireTrailingCommaAtNewline didn't > work very well when I tried it. Ok, perhaps we can't automate/enforce it, but if everyone is agreed on it then we should at least consider it something of a policy and, as you said up-thread, clean things up as we come to them. I'd love to clean up the pg_dump regression tests in such a way to make it simpler to work with in the future, as long as we're agreed on it and we don't end up getting complaints from perlcritic/perltiday or having them end up being removed.. Thanks! Stephen
Attachment
On Sun, May 6, 2018 at 11:53:34AM -0400, Tom Lane wrote:
> What sort of changes do we get if we remove those two flags as you prefer?
> It'd help to see some examples.
>
> Since we just went to a new perltidy version, and made some other
> policy changes for it, in HEAD, it'd make sense to make any further
> changes in this same release cycle rather than drip drip drip over
> multiple cycles. We just need to get some consensus about what
> style we like.
I saw you looking for feedback so I wanted to give mine. Also, Andrew,
thanks for working on this --- it is a big help to have limited Perl
critic reports and good tidiness.
I am using the src/tools/pgindent/perltidyrc setting for my own Perl
code, but needed to add these two:
--noblanks-before-comments
--break-after-all-operators
The first one fixes odd blank lines when I put comments inside
conditional tests, e.g.:
if (!$options{args_supplied} &&
!$is_debug &&
defined($stat_main) &&
defined($stat_cache) &&
$stat_main->mtime < $stat_cache->mtime &&
# is local time zone?
(!defined($ENV{TZ}) || $ENV{TZ} =~ m/^E.T$/))
Without the first option, I get:
if (!$options{args_supplied} &&
!$is_debug &&
defined($stat_main) &&
defined($stat_cache) &&
$stat_main->mtime < $stat_cache->mtime &&
-->
# is local time zone?
(!defined($ENV{TZ}) || $ENV{TZ} =~ m/^E.T$/))
which just looks odd to me. Am I the only person who often does this?
The second option, --break-after-all-operators, is more of a personal
taste, but it does match how our C code works, and people have said I
write C code in Perl. ;-)
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +
On 05/25/2018 03:04 PM, Bruce Momjian wrote:
> On Sun, May 6, 2018 at 11:53:34AM -0400, Tom Lane wrote:
>> What sort of changes do we get if we remove those two flags as you prefer?
>> It'd help to see some examples.
>>
>> Since we just went to a new perltidy version, and made some other
>> policy changes for it, in HEAD, it'd make sense to make any further
>> changes in this same release cycle rather than drip drip drip over
>> multiple cycles. We just need to get some consensus about what
>> style we like.
> I saw you looking for feedback so I wanted to give mine. Also, Andrew,
> thanks for working on this --- it is a big help to have limited Perl
> critic reports and good tidiness.
>
> I am using the src/tools/pgindent/perltidyrc setting for my own Perl
> code, but needed to add these two:
>
> --noblanks-before-comments
> --break-after-all-operators
>
> The first one fixes odd blank lines when I put comments inside
> conditional tests, e.g.:
>
> if (!$options{args_supplied} &&
> !$is_debug &&
> defined($stat_main) &&
> defined($stat_cache) &&
> $stat_main->mtime < $stat_cache->mtime &&
> # is local time zone?
> (!defined($ENV{TZ}) || $ENV{TZ} =~ m/^E.T$/))
>
> Without the first option, I get:
>
> if (!$options{args_supplied} &&
> !$is_debug &&
> defined($stat_main) &&
> defined($stat_cache) &&
> $stat_main->mtime < $stat_cache->mtime &&
> -->
> # is local time zone?
> (!defined($ENV{TZ}) || $ENV{TZ} =~ m/^E.T$/))
>
> which just looks odd to me. Am I the only person who often does this?
>
> The second option, --break-after-all-operators, is more of a personal
> taste, but it does match how our C code works, and people have said I
> write C code in Perl. ;-)
>
I agree with adding --no-blanks-before-comments. That doesn't remove any
blank lines, it just stops perltidy from adding them before comments, so
adding it to the perltidyrc doesn't change anything.
I looked at --break-after-all-operators, but I didn't like the result. I
tried to refine it by adding --want-break-before='. : ? && || and or'.
However, it didn't do what it was told in the case of ? operators. That
seems like a perltidy bug. The bug persists even in the latest version
of perltidy. So I think we should just leave things as they are in this
respect.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services