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

Purge trailing whitespace #7

Merged
merged 10 commits into from Oct 8, 2016

Conversation

Projects
None yet
3 participants
@paultcochrane
Contributor

paultcochrane commented Sep 26, 2016

Trailing whitespace is seen in some projects as bad practice (e.g. the Linux kernel) and is hence explicitly forbidden; other projects see its removal as plain nit-picking. This PR is submitted in the hope that it is helpful, however if you don't see any need to remove such whitespace I'm happy if you close the PR as unmerged. I've split up the PR into several commits so that the diffs are smaller; if you wish for the commits to be collected into one large commit (and thus reduce noise in the git history), just let me know and I'll rebase and force push an update to this branch. If you have any questions or comments concerning the PR, please simply contact me!

@ssimms

This comment has been minimized.

Show comment
Hide comment
@ssimms

ssimms Oct 7, 2016

Owner

It looks like the podchecker PR is conflicting with this one. Can you update it?

Do you know of a way to have this check happen automatically (in Travis, perhaps, or a test?)

Owner

ssimms commented Oct 7, 2016

It looks like the podchecker PR is conflicting with this one. Can you update it?

Do you know of a way to have this check happen automatically (in Travis, perhaps, or a test?)

@paultcochrane

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Oct 7, 2016

Contributor

No worries. I'll rebase this PR, patch and force push where necessary. Have you switched on Travis builds for this repo? You need to go to Travis and set the "build if .travis.yml is present" option for this repo. Then the PRs will also get tested by Travis. Usually GitHub lets you know if a patch conflicts. I'm quite happy to update a patch if it conflicts after a set of changes; it was my patch after all, and I don't want you to go to too much work in merging it :-)

Contributor

paultcochrane commented Oct 7, 2016

No worries. I'll rebase this PR, patch and force push where necessary. Have you switched on Travis builds for this repo? You need to go to Travis and set the "build if .travis.yml is present" option for this repo. Then the PRs will also get tested by Travis. Usually GitHub lets you know if a patch conflicts. I'm quite happy to update a patch if it conflicts after a set of changes; it was my patch after all, and I don't want you to go to too much work in merging it :-)

@ssimms

This comment has been minimized.

Show comment
Hide comment
@ssimms

ssimms Oct 7, 2016

Owner

Yes, Travis is enabled now, but I think it only works on new pull requests / pushes.

Owner

ssimms commented Oct 7, 2016

Yes, Travis is enabled now, but I think it only works on new pull requests / pushes.

@ssimms

This comment has been minimized.

Show comment
Hide comment
@ssimms

ssimms Oct 7, 2016

Owner

I just realized that my earlier comment was ambiguous. Do you know of a way for the trailing whitespace check to happen automatically, so that future commits/PRs can be flagged/rejected if they have unnecessary whitespace?

Owner

ssimms commented Oct 7, 2016

I just realized that my earlier comment was ambiguous. Do you know of a way for the trailing whitespace check to happen automatically, so that future commits/PRs can be flagged/rejected if they have unnecessary whitespace?

Merge branch 'master' into pr/purge-trailing-whitespace
Conflicts:
	lib/PDF/API2/Basic/PDF/Pages.pm
	lib/PDF/API2/Lite.pm
	lib/PDF/API2/Resource/CIDFont/TrueType/FontFile.pm
	lib/PDF/API2/Resource/Font.pm
	lib/PDF/API2/Resource/Font/CoreFont.pm
	lib/PDF/API2/Resource/Font/Postscript.pm
	lib/PDF/API2/Resource/UniFont.pm

Conflicts with current master resolved.
@paultcochrane

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Oct 8, 2016

Contributor

Ah, now I get you. I think the best way to flag that is to add a check for trailing whitespace, perhaps as part of a set of Test::PerlCritic tests. That way Travis will warn you and the PR submitter about the issue (in case they didn't run the tests locally). It's possible to add a commit hook that forbids such commits, however I find that a bit extreme...

Contributor

paultcochrane commented Oct 8, 2016

Ah, now I get you. I think the best way to flag that is to add a check for trailing whitespace, perhaps as part of a set of Test::PerlCritic tests. That way Travis will warn you and the PR submitter about the issue (in case they didn't run the tests locally). It's possible to add a commit hook that forbids such commits, however I find that a bit extreme...

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 8, 2016

Coverage Status

Coverage remained the same at 51.664% when pulling 154bdf5 on paultcochrane:pr/purge-trailing-whitespace into a4eb11a on ssimms:master.

coveralls commented Oct 8, 2016

Coverage Status

Coverage remained the same at 51.664% when pulling 154bdf5 on paultcochrane:pr/purge-trailing-whitespace into a4eb11a on ssimms:master.

@paultcochrane

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Oct 8, 2016

Contributor

I've merged the branch with master and resolved the conflicts and pushed the changes back to the same branch. Dunno if the commits look that clean though; there's a merge commit in there now that doesn't really need to be there. If you want me to clean up the history, just let me know and I'll fix this up and resubmit.

Contributor

paultcochrane commented Oct 8, 2016

I've merged the branch with master and resolved the conflicts and pushed the changes back to the same branch. Dunno if the commits look that clean though; there's a merge commit in there now that doesn't really need to be there. If you want me to clean up the history, just let me know and I'll fix this up and resubmit.

@ssimms

This comment has been minimized.

Show comment
Hide comment
@ssimms

ssimms Oct 8, 2016

Owner

I'll do a "squash and merge" here, since it's just a matter of picking the option in the dropdown. Thanks for resolving the conflicts.

Owner

ssimms commented Oct 8, 2016

I'll do a "squash and merge" here, since it's just a matter of picking the option in the dropdown. Thanks for resolving the conflicts.

@ssimms ssimms merged commit 57743fa into ssimms:master Oct 8, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 51.664%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment