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

fix: pytest 8.1 compat #16

Merged
merged 2 commits into from
Mar 10, 2024
Merged

Conversation

hairmare
Copy link
Contributor

@hairmare hairmare commented Mar 4, 2024

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.

Further 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.

@hairmare hairmare marked this pull request as ready for review March 4, 2024 11:19
@hairmare hairmare changed the title fix: pytest 8 compat fix: pytest 8.1 compat Mar 4, 2024

return RuffFile.from_parent(parent, **make_path_kwargs(path))
return RuffFile.from_parent(parent, **make_path_kwargs(path))
Copy link

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 62.50000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 86.98%. Comparing base (37b8e73) to head (107ecfe).
Report is 2 commits behind head on main.

Files Patch % Lines
pytest_ruff/__init__.py 62.50% 3 Missing and 3 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

@hairmare
Copy link
Contributor Author

hairmare commented Mar 5, 2024

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.

@sbrandtb
Copy link

sbrandtb commented Mar 5, 2024

@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? 🙂

@hairmare
Copy link
Contributor Author

hairmare commented Mar 6, 2024

@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 PYTEST_VER >= (7, 0) and didn't opt for (8, 0)

thanks for approving ci btw, coverage is for sure something that i didn't look into yet and would rather improve

@bluetech
Copy link

Perhaps consider just bumping the pytest dependency to pytest>=7, then you don't need to complicate the code with compat checks. pytest 7.0.0 was released on Feb 4, 2022.

@iurisilvio iurisilvio merged commit 9efc242 into businho:main Mar 10, 2024
5 of 7 checks passed
@hairmare hairmare deleted the fix/pytest-8.1-compat branch March 10, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pluggy._manager.PluginValidationError: Plugin 'ruff' for hook 'pytest_collect_file'
4 participants