Resolve line ending issues #273
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Due to the changes in #244, this has caused a slightly annoying regression for Unix-based users (Linux, MacOS) especially when checking out the code and switching branches. There will always be ghost changes present for certain files (due to the line breaks differences,
LF
on Linux/MacOS andCRLF
on Windows), leading to issues when trying to switch branches (that there are unstaged changes), stash not seeming to work (as the changes automatically appear back), and issues in reverting as mentioned in the Telegram group.The cause of the issue appears to be forcing incompatible line endings, and git will try to "resolve" the issues but proves to be counter-intuitive due to the issues aforementioned.
As such, it will be better to let
git
handle the normalisation of the line endings, to apply the default line ending configuration for text files (which isLF
) by specifying that in.gitattributes
. This will ensure all code checked in to the repository be formatting uniformly with the same line breaks (i.e.LF
).Reasons for choosing this strategy instead of other solutions
Quoting from the blog post in [2]:
Previously mentioned by Cloud7050 Standardize Line Encodings #209 (comment), we should have something easy to implement (i.e. no big codebase changes/commits, ease of use for new users/setup). By setting setting the
text
mode for.gitattribute
, this will automatically normalise the line endings, even if the users does not havecore.autocrlf
setup yet. (refer to the quote below from the StackOverflow post [3])Another comment by shenyih0ng Standardize line encodings #210 (comment) was that we should use
.editorconfig
file to enforce the line endings, however, in that article stated it was mentioned that it was a bonus part and even if it was to implemented, the preferred line style ending would beLF
, and that differs from our current configuration ofCRLF
. As quoted from the blog post [1]:Since we will not be applying eslint rules locally as part of the formatting rules, and we do not really want to enforce working directory (unless there a reason for that, the code checked in to the remotes will be in LF either ways), there does not seem much reason as such and remain highest compatibility with the current system irrespective of whatever operating system you are using.
Type of change
The first commit restores the
.gitattributes
back to auto detection mode without enforcing a specific line ending (and will default to LF), whereas the second commit revert all the files that have been commited to the repo since the merger of #244 to their LF line endings.Merging this PR will close #209 as well.
Checking Line Endings as a Github Action Task
After some consideration on how the behavior of
.gitattributes
, it appears that setting this default value project-wide should be able to ensure uniformity of line endings across the repo, even for new users/contributors without explicitly setting up their git configuration to deal with line endings. (ensures that all code pushed to remote will be LF)There is also additional compute time required to run the linting (via eslint) to check the line breaks, and that if there is any line break issues in a handful number of files, this can easily lead to a memory overflow in the node runtime as eslint only prints out the errors after all the files have been parsed, which could possibly obfuscate the issue (of line breaks) behind memory errors and failing tests.
As such, I have decided against including a check in the Github Actions runner for new pull requests.
Related Issues/PRs & References
Some reference articles used to evaluate the best solution moving forward, and they're definitely worth a good read/look.