-
Notifications
You must be signed in to change notification settings - Fork 10
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: pytest 8.1 compat #16
Conversation
83baff9
to
39afda9
Compare
39afda9
to
d488950
Compare
|
||
return RuffFile.from_parent(parent, **make_path_kwargs(path)) | ||
return RuffFile.from_parent(parent, **make_path_kwargs(path)) |
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.
For the sake of code deduplication, why not:
def _pytest_collect_file(
file_path: Path, parent: pytest.Collector,
) -> RuffFile:
config = parent.config
if not config.option.ruff:
return
if path.ext != ".py":
return
return RuffFile.from_parent(parent, **make_path_kwargs(path))
if PYTEST_VER >= (7, 0):
def pytest_collect_file(file_path, parent):
return _pytest_collect_file(Path(file_path), parent)
else:
def pytest_collect_file(path, parent):
return _pytest_collect_file(path, parent)
For clarity, I also removed unused fspath
and added type hints. My main point is to deduplicate code into a function that looks like the modern version and then call it via a small wrapper for old versions. I feel like this makes it much easier to understand what is going on and what is the actual difference between those two interfaces.
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.
I gave this a try but seem to be stumbling over the make_path_kwargs
function when doing so. I'll have to dig deeper later.
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.
I'd like to reduce duplication too, but it is difficult to make it right, these args are a bit confusing.
I added a tox test with pytest 8.1.0 pinned and it works great.
Released in 0.3.1.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16 +/- ##
==========================================
- Coverage 89.13% 86.98% -2.15%
==========================================
Files 5 5
Lines 138 146 +8
Branches 17 20 +3
==========================================
+ Hits 123 127 +4
- Misses 7 9 +2
- Partials 8 10 +2 ☔ View full report in Codecov by Sentry. |
it looks like the pytest 8.1.0 release was yanked: pytest-dev/pytest#12069 i'll follow up on this once there is a new release available. |
@hairmare Maybe I am misunderstanding, but the change is real and it will happen eventually. The reason why they yanked the release is because they violated their own deprecation policy and the internet was not happy about it 😅 What I am trying to say is that the pathlib based fixture already exists and we can test it with the current version, so when they release a new version that removes the deprecated string-path fixture, we are prepared. Anyway, we are all volunteers and I am totally not trying to convince you to work for me or anything, hence I would offer help if you are short on time or motivation. What do you think? 🙂 |
@sbrandtb consider me convinced anyway ;) the yank did downscale the urgency a bit at my end, but there's a reason i ended up agreeing with the thanks for approving ci btw, coverage is for sure something that i didn't look into yet and would rather improve |
Perhaps consider just bumping the pytest dependency to |
Updates the
pytest_collect_file
hook to conform to the new call syntax while still providing the old syntax for backwards compat with pytest < 7.0.0.stolen frominspired by MNT: Compat with pytest>=8 astropy/pytest-filter-subpackage#15Further notes
The pytest api docs still mention that the deprecated
path
param still works but it was removed in 8.1 completely (pytest-dev/pytest#11757) and the docs will mention it being gone since 8.0.0 once the 8.1.x docs are released.Given the new param was introduced all the way back in 7.0.0, this patch uses it starting from that version. Hopefully this will make removing this, rather kludgy, compat implementation once it isn't needed anymore easier.