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

Fix IO-related perlcritic warnings #10

Merged
merged 2 commits into from Oct 7, 2016

Conversation

Projects
None yet
2 participants
@paultcochrane
Contributor

paultcochrane commented Sep 29, 2016

This PR updates the code to use the three argument form of open and replaces bareword filehandles with scoped lexical variables. These issues were found by perlcritic running at the default severity level. The reason why both issues are handled in the one PR is that the changes often affect the same lines of code, hence if put into separate PRs they would cause conflicts.

This PR is submitted in the hope that it is helpful. If it can be improved upon or needs to be updated, please just let me know and I'll update as required and resubmit.

I noticed a potential issue in lib/PDF/API2/Resource/Font/Postscript.pm at line 118 while preparing this PR: the filehandle is closed on this line, however it is also used in the following elsif block. Unfortunately, I'm not that familiar with the code, so I'm not 100% sure what the correct code would be, however it looks like it would be best to close the filehandle outside the if-elsif-else blocks so that seeks and reads being called in the elsif block run on an opened filehandle. Just thought you'd like to know!

paultcochrane added some commits Sep 29, 2016

Use three-arg form of open
This is considered current best practice, makes the intent more obvious and
avoids subtle bugs when filenames could begin with arbitrary redirection
characters.
Replace bareword filehandles with scoped lexicals
Current best practice recommends that bareword filehandles should be
avoided.
@ssimms

This comment has been minimized.

Show comment
Hide comment
@ssimms

ssimms Oct 7, 2016

Owner

It looks like the if/elsif is parsing two different types of file. In the first case, the file will get closed, but in the second, it doesn't. Moving the close outside of the if seems like the right step, so I'll do that after merging this PR. Thanks for bringing it up.

Owner

ssimms commented Oct 7, 2016

It looks like the if/elsif is parsing two different types of file. In the first case, the file will get closed, but in the second, it doesn't. Moving the close outside of the if seems like the right step, so I'll do that after merging this PR. Thanks for bringing it up.

@ssimms ssimms merged commit 60ed3ae into ssimms:master Oct 7, 2016

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