pgsql: Fix perltidy breaking perlcritic
Fix perltidy breaking perlcritic
perltidying a "##no critic" line moves the marker to where it becomes
useless. Put the line back to how it was, and protect it from further
malfeasance.
Per buildfarm member crake.
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/12d40d4a8d0495cf2c7b564daa8aaa7f107a6c56
Modified Files
--------------
src/backend/catalog/Catalog.pm | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
On Thu, Sep 8, 2022 at 5:23 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:
Fix perltidy breaking perlcritic
perltidying a "##no critic" line moves the marker to where it becomes
useless. Put the line back to how it was, and protect it from further
malfeasance.
A better way do do this IMNSHO is to put the eval in a block on its own
along with the no critic marker on its own line, like this:
{
## no critic (ProhibitStringyEval)
eval ...
}
perlcritic respects block boundaries for its directives.
cheers
andrew
On Fri, Sep 9, 2022 at 3:32 AM Andrew Dunstan <andrew@dunslane.net> wrote:
A better way do do this IMNSHO is to put the eval in a block on its own along with the no critic marker on its own line, like this:
{
## no critic (ProhibitStringyEval)
eval ...
}perlcritic respects block boundaries for its directives.
I tried that in the attached -- it looks a bit nicer but requires more
explanation. I don't have strong feelings either way.
--
John Naylor
EDB: http://www.enterprisedb.com
Attachments:
perlcritic-block-scope.patchtext/x-patch; charset=US-ASCII; name=perlcritic-block-scope.patchDownload
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index 1546e1b335..052abe322f 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -314,11 +314,14 @@ sub ParseData
{
# We're treating the input line as a piece of Perl, so we
# need to use string eval here. Tell perlcritic we know what
- # we're doing.
- #<<< protect next line from perltidy
- # so perlcritic annotation works
- eval '$hash_ref = ' . $_; ## no critic (ProhibitStringyEval)
- #>>>
+ # we're doing. The separate block for this statement is to
+ # limit the scope of the perlcritic exception, which is on
+ # its own line to keep perltidy from reindenting it.
+ {
+ ## no critic (ProhibitStringyEval)
+ eval '$hash_ref = ' . $_;
+ }
+
if (!ref $hash_ref)
{
die "$input_file: error parsing line $.:\n$_\n";
On Fri, Sep 9, 2022 at 10:44 PM John Naylor <john.naylor@enterprisedb.com>
wrote:
On Fri, Sep 9, 2022 at 3:32 AM Andrew Dunstan <andrew@dunslane.net> wrote:
A better way do do this IMNSHO is to put the eval in a block on its own
along with the no critic marker on its own line, like this:
{
## no critic (ProhibitStringyEval)
eval ...
}perlcritic respects block boundaries for its directives.
I tried that in the attached -- it looks a bit nicer but requires more
explanation. I don't have strong feelings either way.
Maybe even better would be just this, which I bet perltidy would not monkey
with, and would require no explanation:
eval "\$hash_ref = $_"; ## no critic (ProhibitStringyEval)
cheers
andrew
[resending to -hackers instead of -committers]
Andrew Dunstan <andrew@dunslane.net> writes:
On Fri, Sep 9, 2022 at 10:44 PM John Naylor <john.naylor@enterprisedb.com>
wrote:On Fri, Sep 9, 2022 at 3:32 AM Andrew Dunstan <andrew@dunslane.net> wrote:
A better way do do this IMNSHO is to put the eval in a block on its own
along with the no critic marker on its own line, like this:
{
## no critic (ProhibitStringyEval)
eval ...
}perlcritic respects block boundaries for its directives.
I tried that in the attached -- it looks a bit nicer but requires more
explanation. I don't have strong feelings either way.Maybe even better would be just this, which I bet perltidy would not monkey
with, and would require no explanation:eval "\$hash_ref = $_"; ## no critic (ProhibitStringyEval)
I didn't see this until it got committed, since I'm not subscribed to
-committers, but I think it would be even better to rely on the fact
that eval returns the value of the last expression in the string, which
also gets rid of the ugly quoting and escaping, per the attached.
- ilmari
Attachments:
0001-Use-return-value-of-eval-instead-of-assigning-inside.patchtext/x-diffDownload
From 8ef12d134e0a21c289796207d87244ba5f5ec92c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 12 Sep 2022 10:43:16 +0100
Subject: [PATCH] Use return value of eval instead of assigning inside string
---
src/backend/catalog/Catalog.pm | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index 919a828ca7..41bbabdfee 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -315,7 +315,7 @@ sub ParseData
# We're treating the input line as a piece of Perl, so we
# need to use string eval here. Tell perlcritic we know what
# we're doing.
- eval "\$hash_ref = $_"; ## no critic (ProhibitStringyEval)
+ $hash_ref = eval $_; ## no critic (ProhibitStringyEval)
if (!ref $hash_ref)
{
die "$input_file: error parsing line $.:\n$_\n";
@@ -361,7 +361,7 @@ sub ParseData
# the whole file at once.
local $/;
my $full_file = <$ifd>;
- eval "\$data = $full_file" ## no critic (ProhibitStringyEval)
+ $data = eval $full_file ## no critic (ProhibitStringyEval)
or die "error parsing $input_file\n";
foreach my $hash_ref (@{$data})
{
--
2.34.1
Andrew Dunstan <andrew@dunslane.net> writes:
On Fri, Sep 9, 2022 at 10:44 PM John Naylor <john.naylor@enterprisedb.com>
wrote:On Fri, Sep 9, 2022 at 3:32 AM Andrew Dunstan <andrew@dunslane.net> wrote:
A better way do do this IMNSHO is to put the eval in a block on its own
along with the no critic marker on its own line, like this:
{
## no critic (ProhibitStringyEval)
eval ...
}perlcritic respects block boundaries for its directives.
I tried that in the attached -- it looks a bit nicer but requires more
explanation. I don't have strong feelings either way.Maybe even better would be just this, which I bet perltidy would not monkey
with, and would require no explanation:eval "\$hash_ref = $_"; ## no critic (ProhibitStringyEval)
I didn't see this until it got committed, since I'm not subscribed to
-committers, but I think it would be even better to rely on the fact
that eval returns the value of the last expression in the string, which
also gets rid of the ugly quoting and escaping, per the attached.
- ilmari
Attachments:
0001-Use-return-value-of-eval-instead-of-assigning-inside.patchtext/x-diffDownload
From 8ef12d134e0a21c289796207d87244ba5f5ec92c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 12 Sep 2022 10:43:16 +0100
Subject: [PATCH] Use return value of eval instead of assigning inside string
---
src/backend/catalog/Catalog.pm | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index 919a828ca7..41bbabdfee 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -315,7 +315,7 @@ sub ParseData
# We're treating the input line as a piece of Perl, so we
# need to use string eval here. Tell perlcritic we know what
# we're doing.
- eval "\$hash_ref = $_"; ## no critic (ProhibitStringyEval)
+ $hash_ref = eval $_; ## no critic (ProhibitStringyEval)
if (!ref $hash_ref)
{
die "$input_file: error parsing line $.:\n$_\n";
@@ -361,7 +361,7 @@ sub ParseData
# the whole file at once.
local $/;
my $full_file = <$ifd>;
- eval "\$data = $full_file" ## no critic (ProhibitStringyEval)
+ $data = eval $full_file ## no critic (ProhibitStringyEval)
or die "error parsing $input_file\n";
foreach my $hash_ref (@{$data})
{
--
2.34.1
On Mon, Sep 12, 2022 at 4:54 PM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:
eval "\$hash_ref = $_"; ## no critic (ProhibitStringyEval)
I didn't see this until it got committed, since I'm not subscribed to
-committers, but I think it would be even better to rely on the fact
that eval returns the value of the last expression in the string, which
also gets rid of the ugly quoting and escaping, per the attached.
Hmm, interesting.
--
John Naylor
EDB: http://www.enterprisedb.com
On 2022-09-13 Tu 05:25, John Naylor wrote:
On Mon, Sep 12, 2022 at 4:54 PM Dagfinn Ilmari Mannsåker
<ilmari@ilmari.org> wrote:eval "\$hash_ref = $_"; ## no critic (ProhibitStringyEval)
I didn't see this until it got committed, since I'm not subscribed to
-committers, but I think it would be even better to rely on the fact
that eval returns the value of the last expression in the string, which
also gets rid of the ugly quoting and escaping, per the attached.Hmm, interesting.
I agree it's a slight stylistic improvement. I was trying to keep as
close as possible to the original.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com