Skip to content

Comments

Add line-endings argument to enforce specific newline format (fix Windows compatibility issues)#57

Open
seaburger wants to merge 4 commits intojumanjihouse:masterfrom
seaburger:add-eol-control
Open

Add line-endings argument to enforce specific newline format (fix Windows compatibility issues)#57
seaburger wants to merge 4 commits intojumanjihouse:masterfrom
seaburger:add-eol-control

Conversation

@seaburger
Copy link
Contributor

There is an issue on Windows when using this formatter when combined with a linter that is configured to accept only \n newlines. When pre-commit-hook-yamlfmt runs, the newlines used for file writes depends on the OS.

Python uses os.linesep by default which means on Windows this is \r\n and on POSIX it is \n
https://docs.python.org/3/library/functions.html#open-newline-parameter

With linter validation running after this formatter, you get stuck in a loop.

  1. You fix line endings to \n in the YAML files and commit
  2. Pre-commit hook runs
  3. yamlfmt rewrites file with \r\n line endings
  4. yamllint fails since it wants \n line endings (with new-lines: enabled)

Example scenario of this happening: netbox-community/devicetype-library#1361 (comment)

This PR adds an optional --line-endings argument which allows overriding this default python behavior. This allows projects to explicitly specify the line ending format they need for their use case and support multiple development platforms.

This is implemented by simply using the newline parameter on the open() call for file writes.

Tests have been added to cover:

  • No --line-endings provided (i.e. all current use of this formatter)
  • Calling with --line-endings lf
  • Calling with --line-endings crlf
  • Calling with --line-endings cr

this allows overriding the default line endings when writing files and
is intended for formatting then linting output in windows environments.
@seaburger
Copy link
Contributor Author

@jumanjiman This PR is now ready to review/merge.

I rebased and updated this code after the other windows compatibility fix in #58 got merged to master.
This PR completes the functionality required to support Windows platform.

If you merge this could you also add a tag 0.4.0 so it can be easily targeted in pre-commit configs for projects that need windows support?

Thanks 😃

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.

1 participant