Automatically renormalize line endings to \n (LF) (Fixes #5315)#5446
Automatically renormalize line endings to \n (LF) (Fixes #5315)#5446jeremyleung521 wants to merge 13 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces repository-wide line-ending normalization by adding a .gitattributes policy and renormalizing existing files to reduce noisy diffs and prevent CRLF-related CI/lint issues (Fixes #5315).
Changes:
- Added
.gitattributesintended to enforce LF line endings. - Renormalized existing repository files (many changes are EOL-only).
- Updated
package/CHANGELOGto record the change.
Reviewed changes
Copilot reviewed 2 out of 94 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| testsuite/MDAnalysisTests/lib/test_log.py | Renormalized line endings (no functional change intended). |
| testsuite/MDAnalysisTests/lib/test_augment.py | Renormalized line endings (no functional change intended). |
| testsuite/MDAnalysisTests/data/windows/WIN_wat.lammpstrj | Renormalized line endings in Windows/DOS fixture. |
| testsuite/MDAnalysisTests/data/windows/WIN_test.arc | Renormalized line endings in Windows/DOS fixture. |
| testsuite/MDAnalysisTests/data/mol2/Molecule.mol2 | Renormalized line endings in test data file. |
| testsuite/MDAnalysisTests/data/mol2/BrokenMolecule.mol2 | Renormalized line endings in test data file. |
| testsuite/MDAnalysisTests/data/gms/symopt.inp | Renormalized line endings in test input file. |
| testsuite/MDAnalysisTests/data/gms/surf2wat.gms | Renormalized line endings in test data file. |
| package/MDAnalysis/lib/_cutil.pxd | Renormalized line endings in Cython declarations. |
| package/MDAnalysis/coordinates/NAMDBIN.py | Renormalized line endings in coordinate reader/writer module. |
| package/CHANGELOG | Added an entry describing the .gitattributes/renormalization work. |
| mypy.ini | Renormalized line endings (config file). |
| mypy_requirements.txt | Renormalized line endings (requirements file). |
| benchmarks/benchmarks/topology.py | Renormalized line endings in benchmark code. |
| .gitattributes | New attributes file to define line-ending policy. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 94 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
.gitattributes:2
- Using
* text eol=lfforces Git to treat all files as text, which can corrupt binary assets by applying EOL normalization. This repo contains many binary files (e.g.,testsuite/MDAnalysisTests/data/adk_dims.dcd,testsuite/MDAnalysisTests/data/surface.trr, and various*.pngdocs images), so this should betext=auto(or explicit binary patterns) instead oftext.
* text=auto eol=lf
package/CHANGELOG:72
- The CHANGELOG entry has minor formatting/wording issues (extra spaces, awkward phrasing, and trailing whitespace). Consider rewording to match surrounding entries and avoid the literal
\nbeing interpreted ambiguously.
* Added `.gitattributes` to enforce LF (\n) as line ending, did a renormalize to
existing files (Issue #5315, PR #5446)
Documentation build overview
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 94 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
package/CHANGELOG:72
- The CHANGELOG entry has awkward phrasing ("did a renormalize") and extra whitespace (double space after ")" and trailing spaces). Rewording it improves clarity and avoids unnecessary whitespace churn.
* Added `.gitattributes` to enforce LF (\n) as line ending, did a renormalize to
existing files (Issue #5315, PR #5446)
IAlibay
left a comment
There was a problem hiding this comment.
May I suggest keeping these changes solely for ".py" files and not everything else? Doing this for binary files (especially TPR files) doesn't seem like a good idea given this isn't "code that was generated with the wrong line endings", but just "how the file actually is".
462057c to
adb26d8
Compare
Done with a new renormalization commit (force pushed). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
.gitattributes:6
- The current attributes mark all files under
testsuite/MDAnalysisTests/data/asbinary, which will also disable text diffs and LF normalization for text-based test data (e.g.,.pdb,.gro,.psf). This makes future reviews/blame harder and undermines the goal of consistent LF endings for text files. Consider only marking known binary trajectory/topology formats asbinaryand leaving text formats as text (so they still geteol=lf).
# Do not touch testsuite data files
testsuite/MDAnalysisTests/data/* binary
testsuite/MDAnalysisTests/data/**/* binary
package/CHANGELOG:72
- This new CHANGELOG entry has trailing whitespace at the end of the lines, which tends to create noisy diffs and can trip whitespace-checking tools.
* Added `.gitattributes` to enforce LF (\n) as line endings and renormalized
existing files to conform (Issue #5315, PR #5446)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 21 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
.gitattributes:7
- The
testsuite/MDAnalysisTests/data/windows/*fixtures are explicitly intended to have DOS/CRLF line endings (seetestsuite/MDAnalysisTests/coordinates/test_windows.pydocstring andtestsuite/MDAnalysisTests/data/windows/README.md). Withtext=auto eol=crlf, Git will normalize these files to LF in the committed blob, so any source archives generated from the repository contents (e.g.,git archive/sdist) will contain LF files and the “DOS line endings” tests/fixtures won’t actually exercise CRLF.
To preserve CRLF in the repository for these fixtures, they should be marked as -text (no normalization) and the files should be re-added with CRLF line endings (or alternatively, adjust the tests to generate CRLF copies at runtime instead of relying on committed CRLF fixtures).
# Tread carefully with testsuite data files
testsuite/MDAnalysisTests/data/* text=auto eol=lf
testsuite/MDAnalysisTests/data/**/* text=auto eol=lf
testsuite/MDAnalysisTests/data/windows/* text=auto eol=crlf
0b9b6c5 to
2bac0c7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 21 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
.gitattributes:7
- The
.gitattributesrules contradict the comment "Do not touch testsuite data files" and also apply CRLF checkout to every file intestsuite/MDAnalysisTests/data/windows/(including__init__.pyandREADME.md). This risks reintroducing CRLF into Python/Markdown sources and makes the intent of the Windows fixtures less clear. Consider removing the redundanttestsuite/MDAnalysisTests/data/*/**/*overrides (the global* text=auto eol=lfalready covers them) and scoping the CRLF rule to the actual Windows fixture files only (e.g.,WIN_*).
# Do not touch testsuite data files
testsuite/MDAnalysisTests/data/* text=auto eol=lf
testsuite/MDAnalysisTests/data/**/* text=auto eol=lf
testsuite/MDAnalysisTests/data/windows/* text=auto eol=crlf
|
All should be well now (bar CI to finish). Latest commits fixes problem where turning everything in data/* as For transparency, there were some problems with the following files which required me to manually remove |
| # Data files from testsuite/MDAnalysisTests/data/windows/* need to end with CRLF for Windows testing | ||
| testsuite/MDAnalysisTests/data/windows/* text=auto eol=crlf |
There was a problem hiding this comment.
Rather leave auto in so any future windows-only test binary files are exempted so no one has to think about it in the future. I don't think text attribute determination is ever documented to have different behaviors across platforms. There were some problems with git <2.10 but that is over 10 years old at this point.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5446 +/- ##
========================================
Coverage 93.85% 93.86%
========================================
Files 182 182
Lines 22510 22510
Branches 3202 3202
========================================
+ Hits 21126 21128 +2
+ Misses 922 921 -1
+ Partials 462 461 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fixes #5315
Regardless of whether #5445 gets merged in due to AI usage or not, the scope of that PR is to check whether bad commits went through. This preemptively deals with problems before commit + combs through existing files. This PR should be merged before #5445 (since this will solve CI failure on that PR anyways) should #5445 be deemed still needed.
Changes made in this Pull Request:
.gitattributesfile to enforce line endings to\n(LF)git add --renormalize .to comb over existing files and implement the.gitattributeschangesWith this PR, when staging a file with CRLF, the following message will be displayed:
warning: in the working copy of 'testsuite/MDAnalysisTests/data/blahblah', CRLF will be replaced by LF the next time Git touches itAnd the changes will be implemented during commit.
LLM / AI generated code disclosure
LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution:
noI guess this is a "yes" now that I read the copilot AI overview on GitHub and implemented changes (not exact) based on what was suggested? Otherwise all done manually (i.e. "no").
PR Checklist
package/CHANGELOGfile updated?package/AUTHORS? (If it is not, add it!)Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.
Other Considerations:
Thought about just listing
.pyand.pyxbut then there's the.cand.hand.iniand.txt(and the list goes on withREADMEandCHANGELOG...). Might as well let git figure it out instead.Also for transparency, there were some problems with the following files which required me to manually remove
\r(don't know why--they were pesky and behavior very much reproducible withgit reset --hard HEAD~1and rerunninggit add --renormalize .) but otherwise should be good now. Rerunninggit add --renormalize .after the manual replacement in a fresh clone does not retrigger this problem so I assume it's a one-time thing.