remove trailing whitespace from Knockout code #260

Merged
merged 13 commits into from Apr 14, 2012

Conversation

Projects
None yet
2 participants
Owner

mbest commented Feb 27, 2012

I have my editor set to remove trailing whitespace, but when I'm working with Knockout code, I turn it off so that I don't "edit" random parts of the code. But if all the code already had the trailing space removed, I wouldn't have to worry about this.

Contributor

SteveSanderson commented Feb 26, 2012

Sure, I'm happy to do this. I propose leaving this to be the last thing done before v2.1.0 is finalized, because otherwise it might lead to more merge conflicts with other outstanding pull requests.

Owner

mbest commented Feb 26, 2012

Yeah, makes sense to wait.

I was also thinking of having the build script check for any files with trailing spaces. On linux, the grep command can do this well:

cd ..
grep -rl ' $' build spec src | grep -v ".exe$"

And on Windows, findstr:

cd ..
findstr -rsm -c:" $" * |findstr -rv "^.git" |findstr -rv ".exe$"

@mbest mbest referenced this pull request Feb 29, 2012

Closed

2.1 release discussion #338

Contributor

SteveSanderson commented Apr 10, 2012

Great stuff, but is there any chance of factoring the new "check for trailing spaces" logic out into separate scripts in the tools subfolder? Just because the build scripts are now getting big enough to need some management of their own.

Tomorrow I'll go through all the files with trailing spaces and remove those trailing spaces.

Owner

mbest commented Apr 11, 2012

Great stuff, but is there any chance of factoring the new "check for trailing spaces" logic out into separate scripts in the tools subfolder?

I'll look into doing that.

Contributor

SteveSanderson commented Apr 13, 2012

Thanks for factoring the whitespace checks out. I've now normalised all the files to eliminate trailing whitespace, and along the way, adapted the build scripts so they will actually fail (refusing to build) if you have unwanted whitespace.

Note that I've eliminated trailing whitespace only from lines that aren't pure whitespace. So:

function somefunc() {
    // The following line is not allowed to have trailing whitespace
    var i = 1;
    // But the following blank line is pure whitespace so it doesn't need to be trimmed. Both Sublime Text and VS put in whitespace to match the current indentation level, so we should allow this.

    return i;
}

What do you think? It's ready to merge as far as I'm concerned. Let me know if you prefer me to do the merge.

Owner

mbest commented on ac96a71 Apr 13, 2012

findstr -rsm -c:"[^ ][ ]$" *.js *.html *.css *.bat *.ps1 > build\%OutTrailingSpaceListFile% works for me in Windows. I don't think we need the extra script step.

Owner

mbest replied Apr 13, 2012

Actually I needed [^ ][ ][ ]*$ to match more than one space.

Owner

mbest commented Apr 13, 2012

Both Sublime Text and VS put in whitespace to match the current indentation level, so we should allow this.

As far as I know, all editors that trim trailing white-space do so on blank lines also. Maybe we can allow them, but have the normalization script strip them.

mbest and others added some commits Apr 13, 2012

Change check scripts so they work exactly the same. Change normaliseA…
…llFiles.ps1 to strip all trailing spaces (even on blank lines).
Amend build checks to disallow trailing whitespace on blank-only line…
…s (so there is now one consistent standard both for normalisation and for build)
Contributor

SteveSanderson commented Apr 14, 2012

Ah, turns out I was wrong - sorry! Sublime Text does not use whitespace to match indentation on blank lines by default. Visual Studio is the only editor I know of that does.

With that in mind, shall we just go with the no-trailing-whitespace-anywhere rule, like you originally suggested? I'd prefer to establish just one standard (both for normalisation and for builds) because otherwise the commit log will get polluted by "I ran the normaliser" commits that merely remove whitespace.

I know people who use VS will still submit pull requests that contain whitespace on blank lines, but we can just politely normalise the changes as part of the merge... :)

Well found! I tried about a dozen ways of expressing this regex - none of which were supported by findstr - but I didn't try that one.

Though now if we trim trailing whitespace everywhere, we won't actually need this anyway. Looks like I wasted a lot of time yesterday :(

Contributor

SteveSanderson commented Apr 14, 2012

Ready to merge now as far as I'm concerned. Again, if you prefer me to do the merge, let me know.

@mbest mbest merged commit cbfc4b9 into master Apr 14, 2012

Owner

mbest commented Apr 14, 2012

Merged. I just found out that you can add w=1 to a commit URL in Github to see only the significant (non whitespace) changes. So here the merge commit showing only the code changes.

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