-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
shadow-dom: Normalize the line endings to LF. #120
shadow-dom: Normalize the line endings to LF. #120
Conversation
This change normalizes the line endings of the files under shadow-dom/ directory to LF, which I believe is standard configuration of Git. In addition, this change adds the .gitattributes file which lets Git deal with newlines automatically. See also: <https://help.github.com/articles/dealing-with-line-endings> No change is made to the content of tests.
LGTM. |
shadow-dom: Normalize the line endings to LF.
I really don't like the idea of git doing anything automatic with newlines. |
I am also very unhappy about the .gitattributes change. In general automatically changing line endings is dubious, and for tests in particular it is extremely bad since they may intend to depend on specific whitespace characters. I also don't like the idea that this directory would have different behaviour to the rest of the repository; that's just leaving a sharp edge for future contributers. I would much prefer to see the opposite patch i.e. a repo-wide configuration to never alter whitespace. |
In theory Git's auto convertion should work fine; personal preference aside, do you have any specific issue with this feature? .gitattributes can prevent people from committing files with random line endings. I think this is an important benefit, and GitHub's documentation also recommends this. (In the past, I even saw a file containing ALL of CRLF, LF and CR line endings!) In case we need whitespace-sensitive tests (I assume this should be rare), you can write a rule in .gitattributes to exclude such tests from being converted. There is no such tests in the shadow-dom directory as far as I know. |
The advantage is that future contributers don't have to know the exact repository configuration, why that might be problematic for certain kinds of tests, and how to change it if it affects their tests. They just get the bytes out that they put in. I really don't think files having a variety of line endings is a real problem, and if it is the right tool to fix it is a text editor, in conjunction with a code review tool, not a VCS. |
I agree with @jgraham here. |
I think the tests where the line endings are significant are very rare, say, less than 1% of whole tests, and probably there is no need in shadow-dom tests. In contrast, the risk that people on Windows commit files with CRLF newlines is relatively high, because: (1) most editors on Windows creates files with CRLF newlines by default; (2) git.autocrlf is not set by default; and thus (3) a contributor who is new to git is likely to commit such files. The fact that many existing files in web-platform-tests have CRLF line endings is a good proof of this theory. I would probably make a mistake, too, when I work on Windows. I think the cost of forcing everyone to understand this and change their environment is unreasonably high, while .gitattributes is a handy way to keep the repository clean most of the time (I'd say, more than 99% of the time) without changing everyone's environment. At this moment we don't have a good alternative other than .gitattributes (GitHub's code review tool doesn't show CRLF, we don't have a presubmit check enforced in this repository, etc). If we really really care about the <1% cases where the line endings are significant, then we can document in README.md or somewhere else about how to exclude the files from getting modified using the .gitattributes file (it's easy). And, I would like to stress that, wrong newline codes do sometimes cause a problem. I have seen some scripts parse CRLF as two newlines multiple times, in different occasions. If there is a known issue where .gitattributes causes a real problem, please let me know. I'd be happy to undo the .gitattributes bits if there is such a problem. Lastly, it seems to me that your argument is purely based on your philosophy that version control systems should not touch the files. However, sometimes touching the files is useful, as I stated above. I really don't like bikeshedding; I appreciate your productive comments. Thank you for reading this long comment. |
I really think we should have this either globally or not at all. Starting to have versioning system behave differently depending on which directory you're in is a recipe for long bug hunting sessions. |
So, if I write a change that fixes the newlines globally, identifies which test is newline-sensitive, adds a .gitattributes in the root directory and adds some information about this in README.md, are you going to approve it? |
Let's just go the easy way, @yutak. Let's remove the .gitattributes and not stress over this :) |
Okay, I'll send a pull request that removes .gitattributes. |
…or_typo Fix typo getting error stack in worker.
Do a full rebuild of the manifest on sync.
This change normalizes the line endings of the files under shadow-dom/
directory to LF, which I believe is standard configuration of Git.
In addition, this change adds the .gitattributes file which lets Git deal with
newlines automatically.
See also: https://help.github.com/articles/dealing-with-line-endings
No change is made to the content of tests.