New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add warnings pragma to all files missing it #15

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@paultcochrane
Contributor

paultcochrane commented 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.

Add warnings pragma to all files missing it
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](http://cpants.cpanauthors.org/release/SSIMMS/PDF-API2-2.028).
@ssimms

This comment has been minimized.

Show comment
Hide comment
@ssimms

ssimms Oct 7, 2016

Owner

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. :-)

Owner

ssimms commented 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

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Oct 8, 2016

Contributor

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.

Contributor

paultcochrane commented 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.

@ssimms

This comment has been minimized.

Show comment
Hide comment
@ssimms

ssimms Aug 18, 2017

Owner

I'm going to close this pull request, per the above conversation. As I'm working my way through the files, I'm gradually replacing the "no warnings" with "use warnings" as I'm updating the code.

Owner

ssimms commented Aug 18, 2017

I'm going to close this pull request, per the above conversation. As I'm working my way through the files, I'm gradually replacing the "no warnings" with "use warnings" as I'm updating the code.

@ssimms ssimms closed this Aug 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment