Skip to content

MAINT: Add missing hooks tests. #653

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

MAINT: Add missing hooks tests. #653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e728f61
MAINT: Add missing hooks tests.
rossbar Oct 18, 2025
ced91b6
TST: Fix hook tests for windows.
rossbar Oct 17, 2025
706ed10
Lint.
rossbar Oct 18, 2025
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
6 changes: 5 additions & 1 deletion numpydoc/tests/hooks/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,11 @@ def test_find_project_root(tmp_path, request, reason_file, files, expected_reaso
(tmp_path / reason_file).touch()

if files:
expected_dir = Path("/") if expected_reason == "file system root" else tmp_path
if expected_reason == "file system root":
expected_dir = Path(tmp_path.drive + tmp_path.root)
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole block edit could instead be

Path(tmp_path.anchor) if expected_reason == "file system root" else tmp_path

Anchor simplifies the addition of the drive and root, I believe.

else:
expected_dir = tmp_path

for file in files:
(tmp_path / file).touch()
else:
Expand Down
13 changes: 13 additions & 0 deletions numpydoc/tests/hooks/test_validate_hook.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Test the numpydoc validate pre-commit hook."""

import inspect
import sys
from pathlib import Path

import pytest
Expand Down Expand Up @@ -63,6 +64,8 @@ def test_validate_hook(example_module, config, capsys):
numpydoc/tests/hooks/example_module.py:30: GL08 The object does not have a docstring
"""
)
if sys.platform == "win32":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option could be string.Template with os.path.sep as a substitute var... I have no preference

Copy link
Contributor

Choose a reason for hiding this comment

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

What about replacing the occurrences of the hardcoded path with str(example_module) using an f-string like this?

- numpydoc/tests/hooks/example_module.py
+ f'{str(example_module)}'

As I understand it pathlib should pick up the separator from the OS, and we already have the path to the example module. Then, we could avoid the replace altogether.

Copy link
Contributor

@mattgebert mattgebert Oct 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

os.sep is better and clearer. Maybe you don't need to even add the conditional; always perform the replacement from the raw string.

Copy link
Contributor Author

@rossbar rossbar Oct 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Both good ideas!

My vote for now would be to get this in as-is with minimal changes to ensure that all the tests are being run, then tackle improvements to the test setup in a followup!

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern with using replace is that it isn't exclusively targeting the file paths in the string. My vote is to use {example_module} to guarantee that even with future changes only the path changes, and we also avoid the conditional. I just confirmed locally (Mac) that this works correctly:

Image

Copy link
Contributor

Choose a reason for hiding this comment

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

I like @stefmolin's suggestion, this also clears the formatting much more.
Don't mind if this is a follow up PR but we should do it.

expected = expected.replace("/", "\\")

return_code = run_hook([example_module], config=config)
assert return_code == 1
Expand Down Expand Up @@ -90,6 +93,8 @@ def test_validate_hook_with_ignore(example_module, capsys):
numpydoc/tests/hooks/example_module.py:30: GL08 The object does not have a docstring
"""
)
if sys.platform == "win32":
expected = expected.replace("/", "\\")

return_code = run_hook([example_module], ignore=["ES01", "SA01", "EX01"])

Expand Down Expand Up @@ -133,6 +138,8 @@ def test_validate_hook_with_toml_config(example_module, tmp_path, capsys):
numpydoc/tests/hooks/example_module.py:30: GL08 The object does not have a docstring
"""
)
if sys.platform == "win32":
expected = expected.replace("/", "\\")

return_code = run_hook([example_module], config=tmp_path)
assert return_code == 1
Expand Down Expand Up @@ -168,6 +175,8 @@ def test_validate_hook_with_setup_cfg(example_module, tmp_path, capsys):
numpydoc/tests/hooks/example_module.py:30: GL08 The object does not have a docstring
"""
)
if sys.platform == "win32":
expected = expected.replace("/", "\\")

return_code = run_hook([example_module], config=tmp_path)
assert return_code == 1
Expand Down Expand Up @@ -209,6 +218,8 @@ def test_validate_hook_exclude_option_pyproject(example_module, tmp_path, capsys
numpydoc/tests/hooks/example_module.py:30: GL08 The object does not have a docstring
"""
)
if sys.platform == "win32":
expected = expected.replace("/", "\\")

return_code = run_hook([example_module], config=tmp_path)
assert return_code == 1
Expand Down Expand Up @@ -242,6 +253,8 @@ def test_validate_hook_exclude_option_setup_cfg(example_module, tmp_path, capsys
numpydoc/tests/hooks/example_module.py:17: PR07 Parameter "*args" has no description
"""
)
if sys.platform == "win32":
expected = expected.replace("/", "\\")

return_code = run_hook([example_module], config=tmp_path)
assert return_code == 1
Expand Down
1 change: 1 addition & 0 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ attr = 'numpydoc.__version__'
[tool.setuptools.package-data]
numpydoc = [
'tests/test_*.py',
'tests/hooks/test_*.py',
'tests/tinybuild/Makefile',
'tests/tinybuild/index.rst',
'tests/tinybuild/*.py',
Expand Down