Anki-Android
Anki-Android copied to clipboard
File terminators not enforced as LF, CRLF can sneak in but should be disallowed
Checked for duplicates?
- [X] This issue is not a duplicate
Does it also happen in the desktop version?
- [X] This bug does not occur in the latest version of Anki
What are the steps to reproduce this bug?
Search code tree for CRLF terminators, or attempt to commit a file that has them:
mike@isabela:~/work/ankidroid/Anki-Android/AnkiDroid/src (main) % for FILE in `find . |grep '\.kt'`; do file $FILE |grep CRLF; done
./androidTest/java/com/ichi2/anki/ReviewerTest.kt: ASCII text, with CRLF line terminators
./test/java/com/ichi2/anki/previewer/PreviewerViewModelTest.kt: ASCII text, with CRLF line terminators
./test/java/com/ichi2/anki/ConstantUniquenessTest.kt: ASCII text, with CRLF line terminators
./main/java/com/ichi2/anki/preferences/AccessibilitySettingsFragment.kt: ASCII text, with CRLF line terminators
./main/java/com/ichi2/anki/pages/ImageOcclusion.kt: ASCII text, with CRLF line terminators
Expected behaviour
There should be no CRLF terminators in the files in the project, additionally, if you attempt to commit any our ktlint rules should fix them and worst case if you commit them anyway it should fail CI via a lint-rules check
Additionally I believe it is possible for git to be configured such that it attempts to fix these as well ?
Actual behaviour
CRLF will happily just go right into the tree
Debug info
doesn't matter as it is infrastructure in the project (ktlint, lint-rules, git settings, etc)
but it's happening as of this typing which is commit hash 74feec0a3dd157d9c3a332f6a3819d2a9154effc which is current main as of June 3rd 2024 @ 11:34a UTC-5
(Optional) Anything else you want to share?
No response
Research
- [X] I am reporting a bug specific to AnkiDroid (Android app)
- [X] I have checked the manual and the FAQ and could not find a solution to my issue
- [X] (Optional) I have confirmed the issue is not resolved in the latest alpha release (instructions)
The desired solution for this should take the shape of:
- investigate git settings to see if we can put something in
.gitattributesrelated to this as a help - investigate ktlint settings to see if there is a setting that will detect CRLFs and disallow them or auto-fix them as part of our commit hook that runs ktlintformat (appears there is an
--ignore-whitespaceswitch in the commit hook so ... unknown what exactly to do there without investigate) - making sure that CI either runs ktlint in a way that enforces no CRLFs as terminators or add a lint-rule that checks it and fails on current code
- a commit that does a
dos2unixon the files that have CRLF terminators - squash merge the result and add that commit on main to the ignore-refs (not entirely sure how to do this, @david-allison knows?)
Note that CRLF terminators may be okay in some file types - this is unknown. But in our main source types (.ts, .gradle, .kt, .xml files at least) I don't think they should be there
I'll be working on it.
squash merge the result and add that commit on main to the ignore-refs (not entirely sure how to do this, @david-allison knows?)
Aim for 1 commit in the PR, but multiple are acceptable
After the commits are in main (and therefore the IDs are stable), edit https://github.com/ankidroid/Anki-Android/blob/a4ac1605ec11cb2272ce7ed5ad4ebf61776ddd6b/.git-blame-ignore-revs and add the additional commits in a follow-up commit
cool @Aditya13s thanks - doesn't have a whole lot to do with coding specifically but issues like this that are codebase-wide standards conformance issues and how to really truly actually provably fix them and make sure they stay fixed are one of those things that becomes important on big projects. So, hopefully useful skills for rest of professional life if you haven't already developed them. And if you already have then should be quick :-)
@mikehardy I added *.kt text eol=lf in .gitattributes which changes crlf to lf, which files are commiting. I never worked with ktlint. Can you help me to start with it.
Sorry I don't have time to help, and I'm not that familiar with it myself