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

Append (not prepend) a newline character(s) to .gitattributes for prettier appearance #5847

Merged
merged 5 commits into from Aug 3, 2021

Conversation

kimsin98
Copy link
Contributor

Current .gitattributes files have this weird structure of an empty line at start and no newline at end. This looks ugly when the files are viewed in terminal.

The solution is very simple and doesn't seem to break anything.

@codecov
Copy link

codecov bot commented Jul 30, 2021

Codecov Report

Merging #5847 (e2faeb9) into master (ee4c809) will decrease coverage by 61.45%.
The diff coverage is 33.33%.

❗ Current head e2faeb9 differs from pull request most recent head 0aff170. Consider uploading reports for the commit 0aff170 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master    #5847       +/-   ##
===========================================
- Coverage   90.62%   29.16%   -61.46%     
===========================================
  Files         310      307        -3     
  Lines       42472    42448       -24     
===========================================
- Hits        38492    12382    -26110     
- Misses       3980    30066    +26086     
Impacted Files Coverage Δ
datalad/support/tests/test_gitrepo.py 0.00% <0.00%> (-99.90%) ⬇️
datalad/tests/test_version.py 0.00% <ø> (-27.78%) ⬇️
datalad/support/gitrepo.py 57.74% <83.33%> (-34.25%) ⬇️
datalad/plugin/wtf.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/addurls.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/tests/test_api.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/no_annex.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/support/digests.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/tests/test_base.py 0.00% <0.00%> (-100.00%) ⬇️
... and 261 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee4c809...0aff170. Read the comment docs.

@kimsin98
Copy link
Contributor Author

Aesthetics aside, it is apparently a bad practice in general to not end with newline. Several command line tools break. There is a reason git often points out missing newlines at EOF.

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall, I am ok with this approach if works as expected (needs a test)

datalad/support/gitrepo.py Outdated Show resolved Hide resolved
datalad/support/gitrepo.py Show resolved Hide resolved
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
@bpoldrack
Copy link
Member

Your last assertion in the test is failing on windows. Need to double-check, but this suggests to me that the line ending handling is wrong. While python3 should by default translate \n properly when open is called with no specified newline option, this likely only applies to read/write methods. You are using seek/tell/truncate instead, but they referring to bytes, not "characters" like \r\n.

So, in your test, your cutting of the \n but not the \r. But the f.read() in your implementation reads the \r and still returns \n (somewhat relaxed auto-translation), failing your condition to add the missing \n. However, we are then reading the file via git config -z where git is responsible for determining what is a line and it only sees a \r.

I haven't double-checked all of that, but in any case: Simply because seek doesn't treat \r\n as a single byte, I'd prefer to only use functions, that include python's auto-translation. So, instead of f.seek(f.tell() -1), just check f.readlines()[-1][-1] == '\n'. If so, prepend the to be written output with an \n and be done.

@kimsin98
Copy link
Contributor Author

kimsin98 commented Aug 3, 2021

I haven't double-checked all of that, but in any case: Simply because seek doesn't treat \r\n as a single byte, I'd prefer to only use functions, that include python's auto-translation. So, instead of f.seek(f.tell() -1), just check f.readlines()[-1][-1] == '\n'. If so, prepend the to be written output with an \n and be done.

read does auto-translate OS-specific newlines, so the problem was f.tell() - 1 assuming 1 char line ending. I fixed that to use len(os.linesep) instead. Also rewrote the test to just strip whitespace at the end.

@bpoldrack
Copy link
Member

That's an option, too - I'm fine with that. :-)

@yarikoptic yarikoptic added enhancement semver-minor Increment the minor version when merged labels Aug 3, 2021
@yarikoptic yarikoptic changed the title prettier gitattributes newlines Append (not prepend) a newline character(s) to .gitattributes for prettier appearance Aug 3, 2021
@yarikoptic
Copy link
Member

thank you @Aksoo , let's proceed and see where it takes us! ;)

@yarikoptic yarikoptic merged commit 1a21a40 into datalad:master Aug 3, 2021
@kimsin98 kimsin98 deleted the gitattribute-newlines branch November 29, 2021 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement semver-minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants