Skip to content
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

Build/PHPCS: improve check for superfluous whitespace in files #542

Merged
merged 3 commits into from Dec 13, 2017

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Dec 2, 2017

Apparently PSR2 turns a couple of the superfluous whitespace checks off.
This turns these back on.

There is one additional check turned off in PSR2 which I've left alone: the check for multiple blank lines in a row.
IMO, sometimes it's useful to use two blank lines to break up code for improved readability, so I'm quite happy to leave that one turned off.

@jrfnl jrfnl added this to the 8.1.0 milestone Dec 2, 2017
@jrfnl jrfnl requested a review from wimg December 2, 2017 17:26
@MarkMaldaba
Copy link
Contributor

MarkMaldaba commented Dec 3, 2017

IMO, sometimes it's useful to use two blank lines to break up code for improved readability, so I'm quite happy to leave that one turned off.

I disagree that it is 'sometimes useful'. This implies that the developer adding the extra blank line is bestowing it with some kind of magic meaning that they expect other developers to understand, e.g. "this next bit is separate to that previous bit" or "now we're going to start generating the output" or whatever. This is not a good way to communicate, as no one else will know what you intended and will most likely assume it is a mistake. Or worse, add code before the double-break when it should be afterwards, or vice-versa, thus completely mangling the intended meaning.

In any situation where you think it would be useful to use multiple blank lines, you should be writing a comment instead.

Whether to turn the sniff on or off is really a question of how nitpicky you want to be, but as we're sniffing for trailing whitespace, which is probably the most nitpicky you can be, it feels that this one should also be turned on.

@wimg
Copy link
Member

wimg commented Dec 3, 2017

I kinda have to agree with Mark here : if it separates blocks of codes for readability, it might be good to either add a comment or split it up in a separate method.

@jrfnl
Copy link
Member Author

jrfnl commented Dec 4, 2017

Ok, maybe that was the dyslectic in me speaking. Most of the time (if not all), there's also a comment.

I'll fix up the commit in a little and fix the places in the codebase which don't comply (probably mostly introduced by me). will ping when done.

@jrfnl jrfnl force-pushed the feature/build-check-superfluous-whitespace branch from 306e10d to 18cb33f Compare December 4, 2017 08:42
@jrfnl
Copy link
Member Author

jrfnl commented Dec 4, 2017

Ok, done. I've fixed up the original commit to include the "multiple empty lines" check and added two more commits:

  1. Realized when adjusting the file that the ruleset used tabs not spaces: fixed.
  2. Fixed violations against the newly enabled check in the existing codebase.

@wimg wimg merged commit c0fd67b into master Dec 13, 2017
@wimg wimg deleted the feature/build-check-superfluous-whitespace branch December 13, 2017 17:02
@jrfnl jrfnl mentioned this pull request Dec 17, 2017
@jrfnl jrfnl added the chores/QA label Mar 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants