search

ssimms / pdfapi2

Create, modify, and examine PDF files in Perl
Other
15 stars 20 forks source link

Purge trailing whitespace #7

Closed paultcochrane closed 7 years ago

paultcochrane commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

ssimms commented 7 years ago

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?

paultcochrane commented 7 years ago

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 commented 7 years ago

Coverage Status

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

paultcochrane commented 7 years ago

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 commented 7 years ago

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.