Skip to content

Add a check for CR/LF in files.#1547

Merged
Anteru merged 3 commits intomasterfrom
task/improve-crlf-handling
Sep 20, 2020
Merged

Add a check for CR/LF in files.#1547
Anteru merged 3 commits intomasterfrom
task/improve-crlf-handling

Conversation

@Anteru
Copy link
Copy Markdown
Collaborator

@Anteru Anteru commented Sep 18, 2020

This can occur when checking out things on Windows, and it breaks the
tarball. This adds a script to check for the presence of CR/LF which
exits early if anything gets found.

This can occur when checking out things on Windows, and it breaks the
tarball. This adds a script to check for the presence of CR/LF which
exits early if anything gets found.
@Anteru Anteru requested a review from birkenfeld September 18, 2020 16:24

for root, dirs, files in os.walk(directory):
for filename in files:
if not filename.endswith('.py'):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hm, this is only checking .py files but the problem was in the bashcomp file...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

As far as I can tell if I mess up the CR/LF I mess it up everywhere. I can exclude *.pyc, I thought this was fairly safe for the particular issue at hand.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added .bashcomp.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense.

Makefile Outdated
all: clean-pyc check test

check:
@$(PYTHON) scripts/check_crlf.py pygments dist || true
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why dist? It contains only tarballs/wheels...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, that should have been build. I've also added external.

* Include the external folder and check that.
* Include .bashcomp files.
* Use the correct CR/LF on the checker itself.
@Anteru
Copy link
Copy Markdown
Collaborator Author

Anteru commented Sep 19, 2020

Good to go? I'd like to merge this followed by #1549 , then I just need to fix too long lines and we could turn on make check for CI.


with open(os.path.join(root, filename), 'rb') as f:
if b'\r\n' in f.read():
sys.exit(1)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do you want to output the name of the offending file?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, good call!

Checker for line endings
~~~~~~~~~~~~~~~~~~~~~~~~

Make sure each Python does not use Windows-style file endings.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

each Python file probably?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

So someone does read the docs! Thanks, good catch.

* Remove || true
* Fix docs
* Print the first offending file name
@Anteru Anteru merged commit 8aa2bc3 into master Sep 20, 2020
@Anteru Anteru deleted the task/improve-crlf-handling branch September 20, 2020 08:24
Kenny2github pushed a commit to Kenny2github/pygments that referenced this pull request Sep 22, 2020
* Add a check for CR/LF in files.

This can occur when checking out things on Windows, and it breaks the
tarball. This adds a script to check for the presence of CR/LF which
exits early if anything gets found.

* Improve error checking.

* Include the external folder and check that.
* Include .bashcomp files.
* Use the correct CR/LF on the checker itself.

* Address review feedback.

* Remove || true
* Fix docs
* Print the first offending file name
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.

2 participants