Thread: pgsql: Perl scripts: eliminate "Useless interpolation" warnings
Perl scripts: eliminate "Useless interpolation" warnings Eliminate warnings of Perl Critic from src/tools. Backpatch-through: master Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/43ce181059d4ecbb1b14b75e7f38a7dda9f80225 Modified Files -------------- src/tools/copyright.pl | 2 +- src/tools/gen_export.pl | 10 +++++----- src/tools/gen_keywordlist.pl | 4 ++-- src/tools/msvc_gendef.pl | 10 +++++----- src/tools/version_stamp.pl | 18 +++++++++--------- src/tools/win32tzlist.pl | 2 +- 6 files changed, 23 insertions(+), 23 deletions(-)
Perl scripts: eliminate "Useless interpolation" warnings Eliminate warnings of Perl Critic from src/tools. Backpatch-through: master
I don't understand this commit. The buildfarm members crake and koel regularly run the perl critic checks and have not complained. See for example from before this change: <https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=koel&dt=2024-09-15%2003%3A34%3A02&stg=perl-check>
The change doesn't seem to have had any discussion either. This particular warning is surely a very low level (i.e. fairly unimportant) one, of the type we normally ignore. If some version of perlcritic has raised it to severity 5 then the correct action IMNSHO would be to add an exception for it to the perlcriticrc, like we do for ProhibitLeadingZeros. If not, then perhaps you can explain how you got the warnings.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On Sun, Sep 15, 2024 at 02:30:47PM -0400, Andrew Dunstan wrote: > > > On 2024-09-15 Su 10:56 AM, Bruce Momjian wrote: > > Perl scripts: eliminate "Useless interpolation" warnings > > Eliminate warnings of Perl Critic from src/tools. > > Backpatch-through: master > > > > I don't understand this commit. The buildfarm members crake and koel regularly > run the perl critic checks and have not complained. See for example from before > this change: <https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm= > koel&dt=2024-09-15%2003%3A34%3A02&stg=perl-check> > > The change doesn't seem to have had any discussion either. This particular > warning is surely a very low level (i.e. fairly unimportant) one, of the type > we normally ignore. If some version of perlcritic has raised it to severity 5 > then the correct action IMNSHO would be to add an exception for it to the > perlcriticrc, like we do for ProhibitLeadingZeros. If not, then perhaps you can > explain how you got the warnings. So, the warning is about the use of double-quotes when single-quotes will work just fine. I wrote a new script and changed it to single quotes, so for consistency, I looked at other Perl scripts that might have the issue. The message I got was: /root/add_commit_links.pl: Useless interpolation of literal string at line 51 near 'my $tmpfile = $file . ".tmp";'. (Severity:1) My $HOME/.perlcritic has: severity = 1 and that is why I saw it. Is it a bad change? -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com When a patient asks the doctor, "Am I going to die?", he means "Am I going to die soon?"
On Sun, Sep 15, 2024 at 02:30:47PM -0400, Andrew Dunstan wrote:On 2024-09-15 Su 10:56 AM, Bruce Momjian wrote: Perl scripts: eliminate "Useless interpolation" warnings Eliminate warnings of Perl Critic from src/tools. Backpatch-through: master I don't understand this commit. The buildfarm members crake and koel regularly run the perl critic checks and have not complained. See for example from before this change: <https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm= koel&dt=2024-09-15%2003%3A34%3A02&stg=perl-check> The change doesn't seem to have had any discussion either. This particular warning is surely a very low level (i.e. fairly unimportant) one, of the type we normally ignore. If some version of perlcritic has raised it to severity 5 then the correct action IMNSHO would be to add an exception for it to the perlcriticrc, like we do for ProhibitLeadingZeros. If not, then perhaps you can explain how you got the warnings.So, the warning is about the use of double-quotes when single-quotes will work just fine. I wrote a new script and changed it to single quotes, so for consistency, I looked at other Perl scripts that might have the issue. The message I got was: /root/add_commit_links.pl: Useless interpolation of literal string at line 51 near 'my $tmpfile = $file . ".tmp";'. (Severity: 1) My $HOME/.perlcritic has: severity = 1 and that is why I saw it. Is it a bad change?
I understand perfectly what the warning is about.
But the project's perlcritic policy is expressed at src/tools/perlcheck/perlcriticrc. It's basically severity 5 plus some additions and one exception. We shouldn't be imposing our personal perlcritic policies on the project.
The change isn't bad in itself, but there shouldn't be any expectation that we will keep to this policy, as it's not required by project policy.
There is a huge mass of perlcritic issues in our perl code at severity 1 (over 13000 by my count). If we're going to clean them up (which I would oppose) we should do it in a systematic way. It's hard to see why this one is special.
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com
On Sun, Sep 15, 2024 at 04:33:49PM -0400, Andrew Dunstan wrote: > I understand perfectly what the warning is about. > > But the project's perlcritic policy is expressed at src/tools/perlcheck/ > perlcriticrc. It's basically severity 5 plus some additions and one exception. > We shouldn't be imposing our personal perlcritic policies on the project. > > The change isn't bad in itself, but there shouldn't be any expectation that we > will keep to this policy, as it's not required by project policy. > > There is a huge mass of perlcritic issues in our perl code at severity 1 (over > 13000 by my count). If we're going to clean them up (which I would oppose) we > should do it in a systematic way. It's hard to see why this one is special. I guess it is special only because my policy is more strict so I wanted my new code to match. Should I revert it? Is the very tiny improvement not worth the churn in the code? -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com When a patient asks the doctor, "Am I going to die?", he means "Am I going to die soon?"
On 2024-09-15 Su 4:36 PM, Bruce Momjian wrote: > On Sun, Sep 15, 2024 at 04:33:49PM -0400, Andrew Dunstan wrote: >> I understand perfectly what the warning is about. >> >> But the project's perlcritic policy is expressed at src/tools/perlcheck/ >> perlcriticrc. It's basically severity 5 plus some additions and one exception. >> We shouldn't be imposing our personal perlcritic policies on the project. >> >> The change isn't bad in itself, but there shouldn't be any expectation that we >> will keep to this policy, as it's not required by project policy. >> >> There is a huge mass of perlcritic issues in our perl code at severity 1 (over >> 13000 by my count). If we're going to clean them up (which I would oppose) we >> should do it in a systematic way. It's hard to see why this one is special. > I guess it is special only because my policy is more strict so I wanted > my new code to match. Should I revert it? Yes I think so. > Is the very tiny improvement > not worth the churn in the code? Yeah, I don't think it is. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Sun, Sep 15, 2024 at 06:07:21PM -0400, Andrew Dunstan wrote:On 2024-09-15 Su 4:36 PM, Bruce Momjian wrote:On Sun, Sep 15, 2024 at 04:33:49PM -0400, Andrew Dunstan wrote:I understand perfectly what the warning is about. But the project's perlcritic policy is expressed at src/tools/perlcheck/ perlcriticrc. It's basically severity 5 plus some additions and one exception. We shouldn't be imposing our personal perlcritic policies on the project. The change isn't bad in itself, but there shouldn't be any expectation that we will keep to this policy, as it's not required by project policy. There is a huge mass of perlcritic issues in our perl code at severity 1 (over 13000 by my count). If we're going to clean them up (which I would oppose) we should do it in a systematic way. It's hard to see why this one is special.I guess it is special only because my policy is more strict so I wanted my new code to match. Should I revert it?Yes I think so.Okay, done.Is the very tiny improvement not worth the churn in the code?Yeah, I don't think it is.FYI, the line that got me started was: my ($tmpfilename) = $filename . ".tmp"; while I would write that with single quotes. Because mine uses single quotes and version_stamp.pl uses double-quotes, I started to try to unify the style.
I would normally write this as
my $tempfile = "$filename.tmp";
or possibly
my $tempfile = "${filename}.tmp";
There's a perl mantra that says There Is More Than One Way To Do It, usually abbreviated to TIMTOWTDI, which perlcritic fights against to some extent. I think the idea of unifying style beyond the official project policy is an effort doomed to failure. Nobody is going to maintain that consistency, and the project has rejected attempts to have a stricter set of rules in the past.
I agree severity=1 generates many false warnings, and changing code based on them can actually produce errors, but I do think there are some safe ones like the single/double quote item we can improve. Attached is my Perl critic file, where I do use severity=1, but turn off various warnings.
Sure, or for something in between, there's the buildfarm policy at <https://github.com/PGBuildFarm/client-code/blob/main/.perlcriticrc>
cheers
andrew
-- Andrew Dunstan EDB: https://www.enterprisedb.com