-
Notifications
You must be signed in to change notification settings - Fork 42
Fix type annotation to allow int keys in data_regression.check() #219
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
Fix type annotation to allow int keys in data_regression.check() #219
Conversation
The method documentation states it accepts 'any yaml serializable dict', but the type annotation was restricted to dict[str, Any]. Since YAML supports integer keys (which are commonly used for test case indexing), this updates the annotation to dict[str | int, Any]. Added test_integer_keys() to demonstrate integer key support. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Thanks @tsvikas The Python 3.9 job failed; we will likely drop support for 3.9 soon, but probably simpler in your case to just update the type annotations for this PR. |
Replace `str | int` with `Union[str, int]` in type annotation to support Python 3.9, where the | operator for unions is not available (only in 3.10+).
|
Done :) |
|
Hi @nicoddemus , just a gentle nudge on this. I’ve updated the type annotations to Union for Python 3.9 compatibility as requested. Let me know if there’s anything else needed to get the CI workflows running! |
|
Thanks for the nudge, this went under my radar! |
| def check( | ||
| self, | ||
| data_dict: dict[str, Any], | ||
| data_dict: dict[Union[str, int], Any], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we might as well just use
| data_dict: dict[Union[str, int], Any], | |
| data_dict: dict[Any, Any], |
here?
We don't have a type for "anything yaml serializable", so might as well use Any and hope for the best.
If you agree, we can also just remove the regression test, seems overkill to have a test for a specific data type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's up to you, ultimately. My logic: if we look at allowed YAML keys (bool, float, str, int, null), then str | int is the reasonable choice because:
strandintare practical and commonly usedboolcan cause confusion (truevs"true",yesvs"yes")floathas precision issues for dict lookupsnullas a key is weird, and don't speaks well with the Python world (Nonevsfloat('na'))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points, lets go with your suggestion then!
|
The CI failure is unrelated to this PR - it's a matplotlib/pyparsing deprecation warning that only appears on Python 3.9. The test is importing matplotlib which uses pyparsing's deprecated oneOf (should be one_of). Since Python 3.9 reached EOL in October 2025, matplotlib/pyparsing likely won't fix it for py39. I suggest that we will approve this PR, and write a separate PR that drop py39 support (remove from tox.ini envlist). alternatively, we can add a filterwarning to ignore this specific deprecation |
Agreed, I will go ahead and merge this. |
Unfortunately the annotation added in #219 caused many regressions. While at it, added type annotations to all tests to catch this kind of regression in the future.
Unfortunately the annotation added in #219 caused many regressions. While at it, added type annotations to all tests to catch this kind of regression in the future.
Unfortunately the annotation added in #219 caused many regressions. While at it, added type annotations to all tests to catch this kind of regression in the future.
Unfortunately the annotation added in #219 caused many regressions. While at it, added type annotations to all tests to catch this kind of regression in the future.
Summary
The
DataRegressionFixture.check()method's type annotation was overly restrictive, only allowingdict[str, Any]when the documentation states it accepts "any yaml serializable dict". This PR updates the type annotation todict[str | int, Any]to allow integer keys.Motivation
Integer keys are commonly used in test data (e.g., for indexing test cases by number), and YAML fully supports them. Users currently have to convert int keys to strings unnecessarily to satisfy type checkers, even though the code works correctly at runtime.
Changes
src/pytest_regressions/data_regression.py:39fromdict[str, Any]todict[str | int, Any]test_integer_keys()test case demonstrating integer key supportType Choice Rationale
We chose
str | intrather than broader types because:trueand"true"Testing
The new test case
test_integer_keys()verifies that integer keys work correctly with the data regression fixture.Real-World Use Case
This fix enables code like: