search

PhilterPaper / Perl-PDF-Builder

Extended version of the popular PDF::API2 Perl-based PDF library for creating, reading, and modifying PDF documents
https://www.catskilltech.com/FreeSW/product/PDF%2DBuilder/title/PDF%3A%3ABuilder/freeSW_full
Other
6 stars 7 forks source link

CTS 15 - use strict; and use warnings; in all modules? #77

Closed PhilterPaper closed 3 years ago

PhilterPaper commented 6 years ago

(from PDF::API2 pull request)

paultcochrane commented on Sep 29, 2016

Using the warnings pragma is considered best practice. By adding the warnings pragma to all files did not cause new warnings to appear in the test output. This change fixes the use_warnings kwalitee test on CPANTS. This PR is submitted in the hope that it is helpful. If it can be improved upon in any way, please simply let me know and I'll update and resubmit it.

<Request is to make sure all .pm files include use warnings;, even if they already have a no warnings list statement. Also, all .pm files should have use strict; to ensure full language compliance. -- Mod.>

ssimms commented on Oct 7, 2016

This one makes me nervous since the test coverage is pretty low. A previous maintainer took the approach of adding "no warnings" to every file, which I've been gradually undoing as I've been updating the code.

Maybe the best way forward is to do a dev release with "use warnings" throughout and see what it does to my production servers. :-)

paultcochrane commented on Oct 8, 2016

Yes, you're right. Either I'd switched off my brain while making this PR or it was late and I was tired... use strict picks things up at compile time, hence the all-usable test will show up those issues, however use warnings only has an effect if the code is run, and hence needs test coverage. If this change is too risky, I don't have a problem if you decide to close it unmerged.

<So, it sounds like use strict; and use warnings; should be inserted into code and run for a while before releasing to the general public? -- Mod.>

PhilterPaper commented 5 years ago

perlcritic at level 4 does a lot of bellyaching about "no warnings" usage. Anything done about warnings and strictness should take care to test against perlcritic to see if its complaints are addressed.

carygravel commented 3 years ago

As long as every bug fix is accompanied by a test case, the lack of test coverage will sort itself out quickly enough. I would introduce this, and every other Perl::Critic warning up to and including severity 1.

PhilterPaper commented 3 years ago

It's too close to the next release (a few weeks, I hope) to add use warnings; and use strict; everywhere it's missing, and try to clean up the resulting blizzard of messages. I'll do that as soon as I roll out 3.020 and then will have time to deal with the mess. Let's get Severity 5 and 4 perl critic cleaned up first, and then worry about any 3, 2, or 1 (I don't think there are any currently). Run-time warnings and errors will of course have to be dealt with before the 3.021 release. I think I'll probably do one module at a time rather than all at once.

PhilterPaper commented 3 years ago

I have removed all 'no warnings' and added 'use warnings', and cleaned up a lot of code (especially uninitialized variables silently treated as 0 or empty string). All my test cases run cleanly, but there's always the chance that someone will stumble upon something that I missed. If so, please open a ticket so I can fix it! This will be released in 3.021.

Kwalitee on CPAN should be much happier with this. There are still a few level 4 and 5 things that Perl Critic complains about, but I'm not sure that it's worth the effort to deal with them. Levels 3, 2, and 1 generate huge numbers of complaints; a few look like it might be reasonable to attack (e.g., variables declared but unused, uninitialized local variables, 'open' and 'eval' return codes ignored). See the comments in tools/1_pc.pl.