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 #590

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .pre-commit-hooks.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@
language: python
types: [text, executable]
stages: [commit, push, manual]
- id: check-illegal-windows-names
name: Check for illegal Windows names
description: Check for files that cannot be created on Windows.
entry: check-illegal-windows-names
language: python
- id: check-json
name: Check JSON
description: This hook checks json files for parseable syntax.
Expand Down
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ Checks for a common error of placing code before the docstring.
#### `check-executables-have-shebangs`
Checks that non-binary executables have a proper shebang.

#### `check-illegal-windows-names`
Check for files that cannot be created on Windows.

#### `check-json`
Attempts to load all json files to verify syntax.

Expand Down
66 changes: 66 additions & 0 deletions pre_commit_hooks/check_illegal_windows_names.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import argparse
import os.path
from typing import Iterable
from typing import Iterator
from typing import Optional
from typing import Sequence
from typing import Set

from pre_commit_hooks.util import added_files
Copy link
Member

Choose a reason for hiding this comment

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

I think it should just filter all files, instead of using added_files here



def lower_set(iterable: Iterable[str]) -> Set[str]:
return {x.lower() for x in iterable}


def parents(file: str) -> Iterator[str]:
file = os.path.dirname(file)
while file:
yield file
file = os.path.dirname(file)


def directories_for(files: Set[str]) -> Set[str]:
return {parent for file in files for parent in parents(file)}


# https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file
ILLEGAL_NAMES = {
Copy link
Member

Choose a reason for hiding this comment

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

frozenset((

'CON',
'PRN',
'AUX',
'NUL',
*(f'COM{i}' for i in range(1, 10)),
*(f'LPT{i}' for i in range(1, 10)),
}


def find_illegal_windows_names(filenames: Sequence[str]) -> int:
relevant_files = set(filenames) | added_files()
relevant_files |= directories_for(relevant_files)
retv = 0

for filename in relevant_files:
root = os.path.basename(filename)
while '.' in root:
root, _ = os.path.splitext(root)
Comment on lines +45 to +46
Copy link
Member

Choose a reason for hiding this comment

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

this can be better written as root, _, _ = root.partition('.')

if root.lower() in lower_set(ILLEGAL_NAMES):
print(f'Illegal name {filename}')
Copy link
Member

Choose a reason for hiding this comment

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

Illegal windows filename: {filename}

retv = 1
return retv


def main(argv: Optional[Sequence[str]] = None) -> int:
parser = argparse.ArgumentParser()
parser.add_argument(
'filenames', nargs='*',
help='Filenames pre-commit believes are changed.',
)

Copy link
Member

Choose a reason for hiding this comment

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

can remove this newline

args = parser.parse_args(argv)

return find_illegal_windows_names(args.filenames)
Copy link
Member

Choose a reason for hiding this comment

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

this can probably be inlined or underscored -- I don't think having a separate function improves anything here (especially once you remove the added_files bit)



if __name__ == '__main__':
exit(main())
1 change: 1 addition & 0 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ console_scripts =
check-case-conflict = pre_commit_hooks.check_case_conflict:main
check-docstring-first = pre_commit_hooks.check_docstring_first:main
check-executables-have-shebangs = pre_commit_hooks.check_executables_have_shebangs:main
check-illegal-windows-names = pre_commit_hooks.check_illegal_windows_names:main
check-json = pre_commit_hooks.check_json:main
check-merge-conflict = pre_commit_hooks.check_merge_conflict:main
check-symlinks = pre_commit_hooks.check_symlinks:main
Expand Down
78 changes: 78 additions & 0 deletions tests/check_illegal_windows_names_test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import sys

import pytest

from pre_commit_hooks.check_illegal_windows_names import (
find_illegal_windows_names,
)
from pre_commit_hooks.check_illegal_windows_names import main
from pre_commit_hooks.check_illegal_windows_names import parents
from pre_commit_hooks.util import cmd_output

skip_win32 = pytest.mark.skipif(
sys.platform == 'win32',
reason='case conflicts between directories and files',
)
Comment on lines +12 to +15
Copy link
Member

Choose a reason for hiding this comment

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

I think once you remove added_files as a constraint you won't need to skip anything (you won't even need to have any disk state either!)



def test_parents():
assert set(parents('a')) == set()
assert set(parents('a/b')) == {'a'}
assert set(parents('a/b/c')) == {'a/b', 'a'}
assert set(parents('a/b/c/d')) == {'a/b/c', 'a/b', 'a'}


def test_nothing_added(temp_git_dir):
with temp_git_dir.as_cwd():
assert find_illegal_windows_names(['f.py']) == 0


def test_adding_something(temp_git_dir):
with temp_git_dir.as_cwd():
temp_git_dir.join('f.py').write("print('hello world')")
cmd_output('git', 'add', 'f.py')

assert find_illegal_windows_names(['f.py']) == 0


@skip_win32 # pragma: win32 no cover
def test_adding_something_with_illegal_filename(temp_git_dir):
with temp_git_dir.as_cwd():
temp_git_dir.join('CoM3.py').write("print('hello world')")
cmd_output('git', 'add', 'CoM3.py')

assert find_illegal_windows_names(['CoM3.py']) == 1


@skip_win32 # pragma: win32 no cover
def test_adding_files_with_illegal_directory(temp_git_dir):
with temp_git_dir.as_cwd():
temp_git_dir.mkdir('lpt2').join('x').write('foo')
cmd_output('git', 'add', '-A')

assert find_illegal_windows_names([]) == 1


@skip_win32 # pragma: win32 no cover
def test_adding_files_with_illegal_deep_directories(temp_git_dir):
with temp_git_dir.as_cwd():
temp_git_dir.mkdir('x').mkdir('y').join('pRn').write('foo')
cmd_output('git', 'add', '-A')

assert find_illegal_windows_names([]) == 1


@skip_win32 # pragma: win32 no cover
def test_integration(temp_git_dir):
with temp_git_dir.as_cwd():
assert main(argv=[]) == 0

temp_git_dir.join('f.py').write("print('hello world')")
cmd_output('git', 'add', 'f.py')

assert main(argv=['f.py']) == 0

temp_git_dir.join('CON.py').write("print('hello world')")
cmd_output('git', 'add', 'CON.py')

assert main(argv=['CON.py']) == 1