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

Revert legacy path removals #12087

Merged
merged 3 commits into from Mar 8, 2024

Conversation

nicoddemus
Copy link
Member

This reverts commits 6c89f92 and a98f02d (as usual, thanks @bluetech for care with the git commit history), bringing back the legacy path arguments to hooks and Node constructors.

We will need to manually fix the changelog for 8.1.1 at the time of release.

Fix #12069

@RonnyPfannschmidt
Copy link
Member

Thanks for picking this up, I'm very grateful this is landing

@bluetech
Copy link
Member

bluetech commented Mar 8, 2024

I know that the py.path hook arguments deprecation didn't work (for implementers, it did work for callers). But are we sure that the py.path node constructor arguments deprecation didn't work? I don't remember seeing any complaints about that one but maybe I missed it.

@nicoddemus
Copy link
Member Author

I know that the py.path hook arguments deprecation didn't work (for implementers, it did work for callers). But are we sure that the py.path node constructor arguments deprecation didn't work? I don't remember seeing any complaints about that one but maybe I missed it.

Good question, I decided to revert both py.path deprecations to avoid confusion -- better to deprecate the py.path arguments (hooks + nodes) in one fell swoop I think.

@bluetech
Copy link
Member

bluetech commented Mar 8, 2024

I think it's better to keep the removal if it was properly deprecated, this way we don't hold up progress in this area until 9.1. But it's OK if you think otherwise.

This PR just undoes the removal, doesn't fix the silent deprecation right?

@nicoddemus
Copy link
Member Author

This PR just undoes the removal, doesn't fix the silent deprecation right?

No it doesn't, we should create a new issue for that.

From #12069 (comment):

the oversight was that warnings of certain hooks where not displayed as expected

@RonnyPfannschmidt do we know exactly for which hooks we were not showing the warnings?

I think it's better to keep the removal if it was properly deprecated, this way we don't hold up progress in this area until 9.1. But it's OK if you think otherwise.

I think it will be less confusing for users, also as I see it the benefit we want is to remove the LEGACY_PATH alias and related code, but having to still support it for hooks we cannot really remove it, so it won't help much removing one but not the other.

@nicoddemus nicoddemus merged commit 2ccc73b into pytest-dev:main Mar 8, 2024
23 of 24 checks passed
@nicoddemus nicoddemus added the backport 8.2.x Apply on merged PRs, backports the changes to the 8.2.x branch label Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 8.2.x Apply on merged PRs, backports the changes to the 8.2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[YANKED] pytest 8.1.0 removes many deprecations, but not mentioned in changelog
3 participants