Follow Perl::Critic::Policy::ValuesAndExpressions rules #2726
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
reosarevok
force-pushed
the
more-perl-critic-valuesandexpressions
branch
6 times, most recently
from
f40cbfb
to
86376c1
Compare
reosarevok
force-pushed
the
more-perl-critic-valuesandexpressions
branch
2 times, most recently
from
63e6621
to
39cd157
Compare
|
@brainzbot, retest this please |
reosarevok
force-pushed
the
more-perl-critic-valuesandexpressions
branch
from
39cd157
to
0745e26
Compare
mwiencek
reviewed
Dec 1, 2023
mwiencek
reviewed
Dec 1, 2023
| unless ($response->code == 200) { | ||
| unless ($response->code == HTTP_OK) { |
There was a problem hiding this comment.
I wrote an awk script to check the diff for this commit (so, if a removed line contained "200", then the next added line should contain "HTTP_OK', etc. for each code/name pair defined in https://metacpan.org/pod/HTTP::Status#CONSTANTS). It didn't work in cases where the line was broken up into multiple, but it turned out that those were the only ones I had to check (other than a couple false positives). Everything else was apparently fine.
mwiencek
reviewed
Dec 1, 2023
| @@ -33,7 +33,7 @@ test 'Area alias appears on alias page content and on JSON-LD' => sub { | |||
|
|
|||
| page_test_jsonld $mech => { | |||
| '@id' => 'http://musicbrainz.org/area/106e0bec-b638-3b37-b731-f53d507dc00e', | |||
| 'alternateName' => ["\x{30aa}\x{30fc}\x{30b9}\x{30c8}\x{30e9}\x{30ea}\x{30a2}"], | |||
| 'alternateName' => ["\N{KATAKANA LETTER O}\N{KATAKANA-HIRAGANA PROLONGED SOUND MARK}\N{KATAKANA LETTER SU}\N{KATAKANA LETTER TO}\N{KATAKANA LETTER RA}\N{KATAKANA LETTER RI}\N{KATAKANA LETTER A}"], | |||
There was a problem hiding this comment.
In this commit, I'm not reviewing any of the changes under t/, because I assume the tests would fail if the wrong characters were used.
mwiencek
reviewed
Dec 1, 2023
reosarevok
force-pushed
the
more-perl-critic-valuesandexpressions
branch
3 times, most recently
from
e8afd30
to
a9b06cd
Compare
mwiencek
requested changes
Dec 4, 2023
reosarevok
force-pushed
the
more-perl-critic-valuesandexpressions
branch
from
a9b06cd
to
15d59e6
Compare
…ents https://metacpan.org/pod/Perl::Critic::Policy::ValuesAndExpressions::ProhibitNullStatements I was honestly surprised to see how many stray semicolons we had, although admittedly it's not hard to forget which blocks take one and which do not. This should help.
…ocTerminator https://metacpan.org/pod/Perl::Critic::Policy::ValuesAndExpressions::RequireUpperCaseHeredocTerminator This is already our consensus, might as well enforce it.
…ocTerminator https://metacpan.org/pod/Perl::Critic::Policy::ValuesAndExpressions::RequireQuotedHeredocTerminator We do this in most places already, and it makes things clearer.
…otelikeOperatorDelimiters https://metacpan.org/pod/Perl::Critic::Policy::ValuesAndExpressions::ProhibitQuotesAsQuotelikeOperatorDelimiters This is just confusing, and we had only two cases. One seemed to be "I dunno what to use, this string uses the defaults, I'll just use q" " - the other, AFAICT, was an actual mistake where qr was meant but qw was used (worked anyway, but :) )
Following https://metacpan.org/pod/Perl::Critic::Policy::ValuesAndExpressions::ProhibitMagicNumbers is a lot of work and there's plenty of cases where it might not even make a lot of sense (such as with IDs in tests). That said, giving the named values for HTTP constants rather than the number (something we already did in many files) is certainly a lot more clear, so I tried to at least make that consistent.
…racters https://metacpan.org/pod/Perl::Critic::Policy::ValuesAndExpressions::ProhibitEscapedCharacters This makes it a lot easier to understand what we are trying to do, especially in tests - now it's also a lot more clear when we are working with defined vs undefined characters.
The only use of all these constants seems to be one call to RELEASE_ATTR_SECTION_STATUS_START in Data::WebService. For now, I just turned that into its own constant on that file, although TBH I'm completely unclear on what that is and why it's used here.
…agma https://metacpan.org/pod/Perl::Critic::Policy::ValuesAndExpressions::ProhibitConstantPragma Although it doesn't seem like Replication.pm intentionally used constant for optimization (https://dev.to/jonasbn/perls-constant-pragma-and-readonly-module-136n) I'm still leaving it as allowed there, since there's a risk changing to Readonly would lead to breaking changes with people's DBDefs that have constants like RT_MIRROR.
reosarevok
force-pushed
the
more-perl-critic-valuesandexpressions
branch
from
15d59e6
to
1943a9e
Compare
mwiencek
approved these changes
Dec 4, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See commit messages for details.
On top of #2725