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

Use strict pragma wherever possible #16

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@paultcochrane
Contributor

paultcochrane commented Sep 29, 2016

... where every module in the distribution now uses the strict pragma.
Only in PDF::API2 was it necessary to turn off strict for 'vars'. The
reason being the options passed around are sometimes handles as arrays and
other times as hashes and it would require lots of digging to sort that
particular issue out. Nevertheless, it was possible to add strict to all
modules where this was missing and to make any variables lexical which
weren't already so. In many cases the new_api method needed to have the
$self variable made lexical and this happened so often, and with the same
structure that it looked like this was a copy-paste issue. Nevertheless,
this variable is now lexical inside new_api throughout the code. The
tests all pass and the Test::Kwalitee::Extra tests no longer complain that
use strict isn't used throughout the distribution. This change should
thus fix the use_strict
CPANTS
violation and allows the distribution to use current Perl best practice.

This PR is submitted in the hope that it is helpful. If it can be improved in any way, please just let me know and I'll update it appropriately and resubmit.

Use strict pragma wherever possible
... where every module in the distribution now uses the `strict` pragma.
Only in `PDF::API2` was it necessary to turn off strict for `'vars'`.  The
reason being the options passed around are sometimes handles as arrays and
other times as hashes and it would require lots of digging to sort that
particular issue out.  Nevertheless, it was possible to add `strict` to all
modules where this was missing and to make any variables lexical which
weren't already so.  In many cases the `new_api` method needed to have the
`$self` variable made lexical and this happened so often, and with the same
structure that it looked like this was a copy-paste issue.  Nevertheless,
this variable is now lexical inside `new_api` throughout the code.  The
tests all pass and the `Test::Kwalitee::Extra` tests no longer complain that
`use strict` isn't used throughout the distribution.  This change should
thus fix the `use_strict`
[CPANTS](http://cpants.cpanauthors.org/release/SSIMMS/PDF-API2-2.028)
violation and allows the distribution to use current Perl best practice.
@ssimms

The new_api cases do seem to be copy-paste errors. Elsewhere, it's setting $obj->{' api'} instead of $self->{' api'}. There were two cases where the scope could matter, so I updated those, and removed the my $self->{' api'} lines from the others.

@@ -12,6 +12,8 @@ use IO::File;
use PDF::API2::Util;
use PDF::API2::Basic::PDF::Utils;
use strict;
no strict 'subs';

This comment has been minimized.

@ssimms

ssimms Oct 7, 2016

Owner

Do you remember why you added this line? Removing it doesn't seem to cause any compile-time errors on 5.8.5, 5.24.0, or 5.22.1.

@ssimms

ssimms Oct 7, 2016

Owner

Do you remember why you added this line? Removing it doesn't seem to cause any compile-time errors on 5.8.5, 5.24.0, or 5.22.1.

@ssimms

This comment has been minimized.

Show comment
Hide comment
@ssimms

ssimms Oct 7, 2016

Owner

I don't see an obvious way to add my local changes to this pull request (I'm relatively new to collaboration with GitHub). If that's possible, let me know how; otherwise, I'll add them in after I hear from you about PNM.pm.

Owner

ssimms commented Oct 7, 2016

I don't see an obvious way to add my local changes to this pull request (I'm relatively new to collaboration with GitHub). If that's possible, let me know how; otherwise, I'll add them in after I hear from you about PNM.pm.

@ssimms

This comment has been minimized.

Show comment
Hide comment
@ssimms

ssimms Oct 8, 2016

Owner

I looked through the issues with strict vars in PDF/API2.pm and was able to eliminate them, so I've merged this pull request manually, including the changes mentioned above, and removing strict subs from PNM.pm.

Thanks for the push to get this done. :-)

Owner

ssimms commented Oct 8, 2016

I looked through the issues with strict vars in PDF/API2.pm and was able to eliminate them, so I've merged this pull request manually, including the changes mentioned above, and removing strict subs from PNM.pm.

Thanks for the push to get this done. :-)

@ssimms ssimms closed this Oct 8, 2016

@paultcochrane

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Oct 8, 2016

Contributor

No worries! Only too pleased to help! The reason why the no strict 'subs' was in that patch was because there were still bareword filehandles floating around, and the only way to get things to compile with use strict is to turn strict off on subs. Of course, since the other patch removing the bareword filehandles was merged before this patch, consequently the no strict 'subs' was no longer necessary.

Am really stoked that you're merging these changes :-) They're mostly trivial/cosmetic changes, however I hope they're useful!

Contributor

paultcochrane commented Oct 8, 2016

No worries! Only too pleased to help! The reason why the no strict 'subs' was in that patch was because there were still bareword filehandles floating around, and the only way to get things to compile with use strict is to turn strict off on subs. Of course, since the other patch removing the bareword filehandles was merged before this patch, consequently the no strict 'subs' was no longer necessary.

Am really stoked that you're merging these changes :-) They're mostly trivial/cosmetic changes, however I hope they're useful!

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