Skip to content

fix whitespace#40

Merged
samrocketman merged 1 commit into
jenkinsci:masterfrom
jhoblitt:maint/fix-whitespace
Jul 13, 2015
Merged

fix whitespace#40
samrocketman merged 1 commit into
jenkinsci:masterfrom
jhoblitt:maint/fix-whitespace

Conversation

@jhoblitt
Copy link
Copy Markdown
Member

@jhoblitt jhoblitt commented Jul 7, 2015

  • expand tabs into spaces
  • fix inconsistent indenting
  • minor formatting changes to improve "style" consistency
  • remove trailing whitespace from end of line
  • remove trailing newline from end of file

@jhoblitt
Copy link
Copy Markdown
Member Author

jhoblitt commented Jul 7, 2015

@samrocketman Unfortunately, expanding just the tabs resulted in monster diff that would be a "rebase barrier" on its own. It also broke mixed tab/whitespace indenting on successive lines and each file needed to be manually inspected.. Since I had to eyeball every line, I went ahead and tried to make the coding style a bit more consistent across files.

@skottler
Copy link
Copy Markdown
Member

skottler commented Jul 7, 2015

I generally support consistency in spacing style, but given massive changes in the history across lots of files like this it's probably better not to merge this. Doing so would make blame essentially useless because of the amount of rewriting that's going on here.

@jhoblitt
Copy link
Copy Markdown
Member Author

jhoblitt commented Jul 7, 2015

Heyas @skottler!

In general, I agree but the mixed tabs/spaces in this plugin are pervasive enough that it complicates PR review and make it painful to edit files with an editor configured to highlight whitespace issues. @samrocketman asked for a reformatting PR in #39 (comment).

@samrocketman
Copy link
Copy Markdown
Member

I don't think blame would be useless. I think someone would use blame and see the whitespace commit. Then use blame again on the commit before whitespace. That would be what I would do. At some point a change like this should be done otherwise it will just remain inconsistently formatted for the lifetime of the project. I'll let others weigh in.

@skottler
Copy link
Copy Markdown
Member

skottler commented Jul 7, 2015

That means that instead of being able to immediately understand the history of a file you have to turn it into a multi-step process, which is less than ideal for not a lot of immediate gain.

At some point a change like this should be done otherwise it will just remain inconsistently formatted for the lifetime of the project.

The whitespace would progressively get fixed as the project changes over time.

Regardless I don't really work on the plugin anymore so if you think it's a good move then go for it.

@sirosen
Copy link
Copy Markdown
Contributor

sirosen commented Jul 7, 2015

I was going to spout a lot of philosophy about whitespace, git-blame, git-log, separating formatting from functionality in commits, and so forth. Ultimately, no one wants to hear that.

What is relevant, however, is this:
I like this way of fixing the whitespace and I favor merging this PR. 👍

@jenkinsadmin
Copy link
Copy Markdown
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@samrocketman
Copy link
Copy Markdown
Member

I have performed a release. I realize it's not really rebase-able due to the amount of white space conflicts. Therefore, I recommend to start over from the latest master. e.g.

git fetch origin
git checkout maint/fix-whitespace
git reset --hard origin/master
#make white space fixes again and commit
git push --force origin maint/fix-whitespace

* expand tabs into spaces
* fix inconsistent indenting
* minor formatting changes to improve "style" consistency
* remove trailing whitespace from end of line
* remove trailing newline from end of file
@jhoblitt jhoblitt force-pushed the maint/fix-whitespace branch from af46ed2 to 02614ad Compare July 13, 2015 20:03
@jhoblitt
Copy link
Copy Markdown
Member Author

@samrocketman I've rebased the non-conflicting files and re-mangled files with changes. That was significantly less fun the second time...

@samrocketman
Copy link
Copy Markdown
Member

Thanks for your time. It was necessary for me to get those other changes out the door for a release. You won't have to do a second rebase because this change is prioritized next. I'll test that this evening and if it all looks good then I'll merge.

@samrocketman samrocketman merged commit 02614ad into jenkinsci:master Jul 13, 2015
samrocketman added a commit that referenced this pull request Jul 13, 2015
@samrocketman
Copy link
Copy Markdown
Member

Successfully reviewed and tested. Thanks for taking the time to correct the white space. I appreciate it.

@jhoblitt
Copy link
Copy Markdown
Member Author

\o/

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants