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

adjust update_envs to make sure that created files have LF line endings #455

Merged
merged 4 commits into from
Apr 9, 2023

Conversation

ChristianZimpelmann
Copy link
Member

  • On Windows, the environment files created by update_envs have CRLF line endings.

  • In particular, this leads to problems with the mixed-line-ending hook.

  • This PR makes sure that LF line endings are used by opening the file in binary mode.

  • I am of course open for other solutions.

@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Merging #455 (88049f3) into main (6d762a5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #455   +/-   ##
=======================================
  Coverage   93.24%   93.24%           
=======================================
  Files         247      247           
  Lines       17987    17987           
=======================================
  Hits        16772    16772           
  Misses       1215     1215           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@janosg
Copy link
Member

janosg commented Apr 6, 2023

Hi @ChristianZimpelmann, can you describe the problem you get with the mixed-line-ending hook? I thought the whole point of that hook was to fix such problems.

@ChristianZimpelmann
Copy link
Member Author

Hi @ChristianZimpelmann, can you describe the problem you get with the mixed-line-ending hook? I thought the whole point of that hook was to fix such problems.

The problem is that mixed-line-ending and update-environment-files fail alternatingly and alternatingly change the line endings from LF to CRLF and viceversa. Somehow mixed-line-ending does not check the files created by update-environment-files during the same pre-commit run, but only after I try to commit again.

@janosg
Copy link
Member

janosg commented Apr 7, 2023

Ah, probably because they are not staged.

I was just worried how robust this approach is. Do you have a source for this trick? Then we could just add a comment and the source to explain why we open a yaml file in binary mode.

@ChristianZimpelmann
Copy link
Member Author

Good point!

I found a much better solution now, but nevertheless added a comment including a source for the solution.

@timmens
Copy link
Member

timmens commented Apr 7, 2023

Our development environment already uses Python 3.10, so we could probably also use this, right?

Path('my.txt').write_text('hello\nworld', newline='\n')

Source: https://stackoverflow.com/a/69869641

Copy link
Member

@janosg janosg left a comment

Choose a reason for hiding this comment

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

We can merge this from my side (as is or with Tim's suggestion). Thanks a lot!

@ChristianZimpelmann
Copy link
Member Author

Thanks, @timmens for the reference! That seems like the best solution.

@ChristianZimpelmann ChristianZimpelmann merged commit a770938 into main Apr 9, 2023
@ChristianZimpelmann ChristianZimpelmann deleted the fix_crlf branch April 9, 2023 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants