Skip to content

Critic#135

Closed
carygravel wants to merge 2 commits intoPhilterPaper:masterfrom
carygravel:critic
Closed

Critic#135
carygravel wants to merge 2 commits intoPhilterPaper:masterfrom
carygravel:critic

Conversation

@carygravel
Copy link
Contributor

Let's standardize the Perl Critic tests, put the resource file where it can be found, and then turn up the severity.

@PhilterPaper
Copy link
Owner

I normally run tools/1_pc.pl from the top level (where .perlcriticrc lives). It already uses level 4 with exclusion of certain common need-to-be-dealt-with messages. How do your changes affect that? I've never done anything with t/author-critic.t because it's always been skipped. I haven't looked at whether I can set an environment variable like AUTHOR_TESTING in Windows.

@PhilterPaper
Copy link
Owner

This test (author-critic.t) is functionally a subset of tools/1_pc.pl, so I have decided not to pull it. I did go in and update where it finds the configuration file (../.perlcriticrc instead of perlcritic.rc), but did not move it to t/ or update the severity from 5 to 4 (1_pc does use severity 4, and filters out nuisance warnings). When run from the top level, 1_pc will criticize everything Perl in lower directories, including contrib, examples, and tools; in addition to lib, whereas these directories won't normally be installed in a package installation anyway (lib/ only), so there's really no point in having them in a t-test.

@carygravel
Copy link
Contributor Author

I completely understand that you prefer to run the author tests with tools/1_pc.pl. Given that is non-standard, it would be good to make sure that tools/1_pc.pl and t/author-critic.t do exactly the same thing.

I think it is worthwhile criticising all the code - if the code isn't good, surely it's not worth having?

@PhilterPaper
Copy link
Owner

All code is criticized by 1_pc.pl, so that box is checked. Note that at severity 4, it gets a lot of "nuisance" warnings that should be dealt with some day, but I just haven't gotten around to it. For now, they're just filtered out so they don't clutter up the report and make it easy to overlook something serious. I try to fix the simple (or serious) stuff when it shows up.

Given that for a typical (non-developer) install, contrib/, examples/, and tools/ won't be installed anyway, I don't find author-critic.t to be all that useful. 1_pc.pl is a superset of its abilities, and I run 1_pc.pl before final tests and packaging up for CPAN. I'll leave author-critic.t in the t-tests in case someone wants to enable AUTHOR_TESTING and run it, but otherwise I don't intend to pursue the matter. Just remember to change the SEVERITY setting from 5 to 4 if you want to do anything serious with author-critic.t.

@PhilterPaper
Copy link
Owner

See also #77, which refers to ssimms/pdfapi2#15 and ssimms/pdfapi2#16.

@PhilterPaper
Copy link
Owner

I went ahead and bumped up the severity setting to 4, as tools/1_pc.pl doesn't use this file.

@PhilterPaper
Copy link
Owner

An-n-n-n-nd... I had to set SEVERITY back to 5, because something in test.yaml's lint testing is using .perlcriticrc, and blowing up badly at SEVERITY 4. :-(

@carygravel carygravel deleted the critic branch October 25, 2021 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants