Problem/Motivation
Some files containing code in core have trailing whitespace which doesn't match with our coding standards: https://www.drupal.org/coding-standards#indenting
Proposed resolution
Remove trailing whitespace from core code. Don't touch anything in vendor or .txt files.
Remaining tasks
None. The results of the patch can be verified with ack 2.x:
ack "[ \t]+$" core --ignore-dir=core/vendor --ignore-dir=core/assets --ignore-file=match:.*\.txt
User interface changes
None
API changes
None
Original report by @pillarsdotnet
While submitting unrelated patches, I have noticed several core files that have trailing whitespace. This can easily be cleared up via the following one-line command:
perl -pi -e 's, *$,,' $(find core -regex '.*\.\(engine\|html\|inc\|info\|install\|js\|json\|module\|php\|script\|sh\|sql\|test\|txt\|xml\)')
Comment | File | Size | Author |
---|---|---|---|
#46 | interdiff-1419298-41-46.txt | 14.13 KB | ryan.gibson |
#46 | 1419298-remove-trailing-whitespace-46.patch | 1.11 KB | ryan.gibson |
#41 | interdiff-1419298-40-41.txt | 1.48 KB | zaporylie |
#41 | 1419298-remove-trailing-whitespace-41.patch | 20.81 KB | zaporylie |
#40 | 1419298-remove-trailing-whitespace-40.patch | 20.78 KB | ohthehugemanatee |
Comments
Comment #1
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedGreat. Maybe it would also be possible to remove trailing whitespace from patch files uploaded to Drupal.org. But that's for another issue queue...
Comment #2
pillarsdotnet CreditAttribution: pillarsdotnet commentedPatch.
Comment #3
pillarsdotnet CreditAttribution: pillarsdotnet commented(sigh) For real, this time.
Comment #4
pillarsdotnet CreditAttribution: pillarsdotnet commented@#1 remove trailing whitespace from patch files
Super-terrific-wonderful-great idea for a 6.x/7.x contrib module.
Comment #5
jhodgdonHm. I'm all for this patch, but I'm wondering if we should be changing
core/modules/filter/tests/filter.url-input.txt and
core/modules/filter/tests/filter.url-output.txt
I guess all of the tests passed, but I also noticed there is no newline at the end of these files, so maybe the extra spaces were intentional in some way? I'm not sure, and I would say maybe we should avoid changing those files.
Comment #6
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-rolled as requested.
Comment #7
jhodgdonThanks! I think this is ready to commit.
Comment #8
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedYes, looks good. This has no chance of messing up anything, so safe fix!
Comment #9
pillarsdotnet CreditAttribution: pillarsdotnet commentedRegenerated after multiple core commits, but the resultant patch is identical to the one in #6.
(bump!)
Comment #10
jhodgdon#6: remove-trailing-whitespace-1419298-6.patch queued for re-testing.
Comment #11
jhodgdonJust a note that this affects much more than just .txt files and more than just documentation. It probably cannot be committed until the bug thresholds fall though.
Comment #12
webchickWe're under thresholds so this is back on the table again. Assigning to Jennifer, and sending for a re-test.
Comment #13
webchick#6: remove-trailing-whitespace-1419298-6.patch queued for re-testing.
Comment #14
webchickI should mention though that my /vast/ preference is to have things like this handled through http://drupal.org/project/drupalcs so they can be caught *before* the patch is uploaded to d.o.
Comment #16
jhodgdonUnassigning so someone can reroll this patch.
Comment #17
pillarsdotnet CreditAttribution: pillarsdotnet commentedRe-rolled.
Comment #19
pillarsdotnet CreditAttribution: pillarsdotnet commentedUnsubscribing. Since there are only twelve lines changed, and I can't imagine, after a thorough review of the patch, why it would break core, I find that I just don't care anymore.
Comment #20
jhodgdonJust because you're frustrated is not necessarily a good reason to close the issue. Sigh.
Comment #21
pmitchell CreditAttribution: pmitchell commentedRerolling (working on as part of DC2012 core sprints)
Comment #22
BrockBoland CreditAttribution: BrockBoland commented#17: remove-trailing-whitespace-1419298-17.patch queued for re-testing.
Comment #23
pmitchell CreditAttribution: pmitchell commentedGenerated by the command in opening post (against the current HEAD), excludes filter/tests/filter-input-* things (which intentionally include trailing whitespace).
Comment #24
BrockBoland CreditAttribution: BrockBoland commentedI applied the patch in #17 to an up-to-date checkout of 8.x and ran all tests, and it came back clean. Is there any chance there was another bug in 8.x that could have caused the tests to fail on this patch?
Re-queued for testing in hopes that it comes back OK this time. It sounds like pmitchell is re-creating the patch against the current HEAD, so I'll leave that to him/her.
Comment #25
BrockBoland CreditAttribution: BrockBoland commentedNot sure why pmitchell's worked and pillarsdotnet's did not, but this latest patch looks good to me and passes tests.
Comment #26
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedI know this one is marked to remove trailing whitespace but what about tabs? In particular the misc folder has quite a few files that have tabs instead of spaces.
I didn't know if I should open a new issue or not so please let me know.
Comment #27
Lars Toomre CreditAttribution: Lars Toomre commentedThe current patch is a monster. To avoid making it even bigger, I would recommend handling replacing tabs with spaces in a new issue so that this one can be put to bed.
Comment #28
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedSetting this back to RTBC. The new issue is #1513540: Replace tabs with spaces from Drupal core files..
Comment #29
catch#26: remove-trailing-whitespace-1419298-26.patch queued for re-testing.
Comment #31
NROTC_Webmaster CreditAttribution: NROTC_Webmaster commentedReroll of #22
Comment #32
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLooks like this doesn't apply anymore. #31: remove-trailing-whitespace-1419298-31.patch queued for re-testing.
Btw.
git diff --check $(git hash-object -t tree /dev/null)
is another nice way to check for whitespace issues.Comment #34
Albert Volkman CreditAttribution: Albert Volkman commentedIt looks like a lot of these have already been cleaned up. Found a few more as well. I see there's some trailing whitespace in Doctrine, Symfony and Twig vendor libraries... do we address those or leave them as-is?
Comment #35
droplet CreditAttribution: droplet commentedso far only few places, it doesn't hack 3rd party scripts.
Comment #36
ramlev CreditAttribution: ramlev commentedHave removed all trailing spaces and tabs from complete core
Comment #37
webchickThanks, but let's hold off on this until after feature freeze so we don't invalidate peoples' patches in the middle of panic-session. And I would much prefer to just have a script that the core committers can run themselves (or ideally, could be run automatically pre-commit) rather than committing 1.85M patches. :D
Comment #38
mgiffordafter feature freeze.
Comment #39
jsobiecki CreditAttribution: jsobiecki commentedComment #40
ohthehugemanatee CreditAttribution: ohthehugemanatee commentedwooo! 543 line patch that just removes the ends of lines!
FWIW that perl command didn't run on OSX, because Apple. Had to run it in my vagrant environment. :)
Comment #41
zaporylieI fixed missed line endings in two txt files and trailing whitespaces in other one. I've attached interdiff to #40.
Comment #42
zaporylieComment #43
mgiffordI applied the patch, reviewed the code and am happy with the results. May this get in before a bunch of other things change.
Comment #44
alexpottThis is not our code. We should not be changing it.
And neither is any of this. We should only be touching code that is not in a vendor directory.
This is not code and should not have coding standards applied to it.
So we only have 2 instances in the whole of core where we have whitepsace incorrectly at the end of a line. Amazing.
Comment #45
ryan.gibson CreditAttribution: ryan.gibson commentedSnatching this before someone else does. :)
Comment #46
ryan.gibson CreditAttribution: ryan.gibson commentedAlright, here's the patch minus the non-code files and vendor directories. :)
Comment #47
star-szrBefore patch:
After patching, no results. I think we're good to get this in and move on :)
Comment #48
star-szrAlso if this isn't minor I don't know what is :D
Comment #49
webchickOk, let's get 'er done. :)
Committed and pushed to 8.0.x. Thanks!