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

meta: add action that requires LF line endings #956

Merged
merged 2 commits into from
May 28, 2021
Merged

meta: add action that requires LF line endings #956

merged 2 commits into from
May 28, 2021

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented May 27, 2021

What

Add action that requires LF line endings, and errors if CRLF line endings are present in CAPs.

Why

Different line endings are inconvenient when folks on different operating systems are working together on documents. I found this when working on CAP-35 but ignore it at the time. Discovered in #950 that the CRLF line endings can affect diffs.

Close #953

Known Limitations

This action is limited to checking CAPs as a starting point. We can extend it to SEPs too, but let's start here.

@leighmcculloch
Copy link
Member Author

This test will pass once #952 is merged.

@leighmcculloch leighmcculloch marked this pull request as ready for review May 28, 2021 01:23
steps:
- run: sudo apt-get update && sudo apt-get install dos2unix
- uses: actions/checkout@v2
- run: (ls core/cap-*.md | xargs -I '{}' sh -c 'cat {} | dos2unix | cmp - {}') || (echo 'Documents listed above contain CRLF line endings, and should be LF.'; exit 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to enforce encoding or crlf is enough?

I see that dos2unix allows to enforce utf8bom for example.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to enforce both, but I'd like to start with only line endings in this PR and follow up with utf8 in a separate pr once I understand the state of that better I'm the repo.

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked more at this, I don't think we need to enforce utf8bom, just utf8 is sufficient whether the bom is present or not. Many of our CAPs only used ASCII characters and so the distinction is not particularly useful. If any file strays outside utf8 the dos2unix tool should error on that I think.

Unless you think we should require utf8bom, or need to do more testing here, I think we're okay.

@leighmcculloch leighmcculloch merged commit f999b6e into stellar:master May 28, 2021
@leighmcculloch leighmcculloch deleted the meta-require-lf-line-endings branch May 28, 2021 22:04
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.

meta: add check that requires documents are eol=lf when committed, and UTF-8
2 participants