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

Add check for illegal Widnows names #1031

Merged
merged 1 commit into from
Apr 14, 2024

Conversation

ericfrederich
Copy link
Contributor

Fixes #589

@ericfrederich ericfrederich force-pushed the check-illegal-windows-names branch from 17efd6f to 994a148 Compare March 26, 2024 20:50
@ericfrederich ericfrederich changed the title WIP: Add check for illegal Widnows names Add check for illegal Widnows names Mar 26, 2024
@ericfrederich
Copy link
Contributor Author

Took advice from #590 and created a new PR ... sorry for delay

name: check illegal windows names
entry: Illegal windows filenames detected
language: fail
files: '(?i)(^|/)(CON|PRN|AUX|NUL|COM[1-9]|LPT[1-9])(\.|/|$)'
Copy link
Member

Choose a reason for hiding this comment

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

can we add a quick little test which loads this regex from the configuration and runs it against a few expected values?

Choose a reason for hiding this comment

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

@asottile @ericfrederich I recommend checking for the : character as well (see my comment in #589 regarding :Zone.Identifier files).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jleclanche looks like this got merged without code checking for the : but no release has been made yet.

I just checked and it's definitely a problem on Windows. I created a file foo:bar on linux, commited, pushed, then pulled down the repo from Windows and got greeted with error: invalid path 'foo:bar'

I'll add a test for it and a new PR

@ericfrederich ericfrederich force-pushed the check-illegal-windows-names branch 2 times, most recently from 644db3e to f2915d1 Compare April 10, 2024 19:21
@ericfrederich
Copy link
Contributor Author

Tests added, let me know if that's what you had in mind.

@asottile asottile force-pushed the check-illegal-windows-names branch from f2915d1 to 8654097 Compare April 14, 2024 15:47
Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

@asottile asottile enabled auto-merge April 14, 2024 15:48
@asottile asottile merged commit 0d20f18 into pre-commit:main Apr 14, 2024
6 checks passed
ericfrederich added a commit to ericfrederich/pre-commit-hooks that referenced this pull request Apr 16, 2024
@thetillhoff
Copy link

Any plans on releasing a new version soon with this check included?

Just read about it on the Webpage, tried to add it, but it wasn't found. Then checked the dates, and the last release of pre-commit was before this PR was merged...

@asottile
Copy link
Member

asottile commented Jul 5, 2024

you're confused?

@thetillhoff
Copy link

thetillhoff commented Jul 5, 2024

Am I?
This PR adds the check-illegal-windows-names hook, and was merged on April 16th.
The last released version of this repo (https://github.com/pre-commit/pre-commit-hooks/releases/tag/v4.6.0) was released on April 6th.

Is there a plan to create a new release that includes the check-illegal-windows-names hook?
Or am I misunderstanding something?

EDIT: And I came across it while reading through https://github.com/pre-commit/pre-commit-hooks where said hook is already listed.

@asottile
Copy link
Member

asottile commented Jul 5, 2024

have you read the pre-commit docs?

@thetillhoff
Copy link

Probably not the part you have in mind. You are welcome to point me to what you want me to read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add check for illegal Windows filenames
4 participants