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

Whitespace cleanup #174

Merged
2 commits merged into from Oct 30, 2010
Merged

Whitespace cleanup #174

2 commits merged into from Oct 30, 2010

Conversation

ddale
Copy link
Contributor

@ddale ddale commented Oct 18, 2010

some files crept in with windows line endings. We should encourage people to add the following to their .gitconfig files:

[apply]
    whitespace = fix
[core]
    autocrlf = input

@fperez
Copy link
Member

fperez commented Oct 21, 2010

Thanks for the suggestion on the .gitconfig file, but I'm not 100% sure we want that:

  • For files that are meant to be used natively on Windows, keeping cr/lf line endings may be the right thing to do. Anyone editing those from *nix will have a capable editor, but someone on Windows may be using notepad, which will get utterly confused if the file has only \n
  • Blanket replacement of line endings on files like SVGs may also not be OK: the svg your commit modifies was actually created on a Mac with Adobe Illustrator. If that tool produces natively windows line endings, spurious conversions of line endings on all commits are just likely to generate more noise than they are worth.

That's at least my impression...

@ddale
Copy link
Contributor Author

ddale commented Oct 21, 2010

We had some discussion about a similar issue at numpy-discussion. Chuck Harris found documentation for the svg format stating that LF is the expected line ending.

There is some discussion at http://www.kernel.org/pub/software/scm/git/docs/gitattributes.html . See also: http://help.github.com/dealing-with-lineendings/ , http://www.kernel.org/pub/software/scm/git/docs/git-config.html . It looks like the appropriate thing to do is to commit a .gitattributes file along with the existing .gitignore. In that file:

* text=auto
my_native_windows_file.argh eol=crlf

that way git will attempt to convert all text files to LF, with the exception of any files that are explicitly marked for committing with alternative eols. Files in the working directory will generally still have native file endings (see core.eol and core.autocrlf in the gitconfig documentation).

@fperez
Copy link
Member

fperez commented Oct 22, 2010

I'm not too up to speed on those details, but I trust you've followed the minutia on the list, and your description above sounds very reasonable.

If you can update this branch with the attributes file you mention as above, I have no problem making the merge, unless someone objects.

While whitespace cleanups aren't in general my top priority, if we do it once in an isolated commit and especially if we leave the git setup in more robust form for the future, I have no objection.

@fperez
Copy link
Member

fperez commented Oct 26, 2010

Hi Darren, just checking: do you want to update the pull request with the necessary .gitattributes file? I haven't followed this enough to know precisely what needs to be put in the file (I'm not sure your text in the box above is meant as the actual file contents or just an indication of what would go there).

If you do the update, we can merge and close this.

Thanks!

@ddale
Copy link
Contributor Author

ddale commented Oct 27, 2010

I just got really busy at work, and won't have time to deal with it for a few days. The discussion on the numpy mailing list has become noisy to the point of static. Let's see how it plays out and I'll post a new pull request if warranted.

@fperez
Copy link
Member

fperez commented Oct 27, 2010

No problem, I'll leave this one in your hands and trust you. We can give it a few weeks, and when you have a bit of bandwidth we'll decide whether to update it or just close it.

@ellisonbg
Copy link
Member

The changes in the existing commits are totally fine, but I guess we need to wait until the .gitattributes stuff is addressed.

@ddale
Copy link
Contributor Author

ddale commented Oct 29, 2010

I think I'll have time to tie this off this weekend.

@fperez
Copy link
Member

fperez commented Oct 30, 2010

Hey Darren,
I just wanted to give you a quick heads-up. I just merged 133507f, which adds a .gitattributes as well (for a different purpose). So you may have a bit of a conflict there, hopefully easy to sort out. Let me know if you need a hand, and sorry if it causes you any hassles.

@ddale
Copy link
Contributor Author

ddale commented Oct 30, 2010

I'll just file a new pull request.

@ddale
Copy link
Contributor Author

ddale commented Oct 30, 2010

Huh. I deleted the original whitespace-cleanup remote, made a new branch off master, applied changes, pushed again to a new remote called whitespace-cleanup, and now this pull request reflects the new changes on the new remote.

@fperez
Copy link
Member

fperez commented Oct 30, 2010

OK, I'll merge it now; the merge commit will close it.

Thanks a lot for your patience with the review/feedback!

This pull request was closed.
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.

None yet

3 participants