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

Deprecate implprefix #144

Merged
merged 3 commits into from
May 22, 2018
Merged

Conversation

goodboy
Copy link
Contributor

@goodboy goodboy commented May 18, 2018

Pertains to starting work on #116.
I'll have a corresponding PR to pytest to ensure the warning is avoided - turned out it's a trivial change.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Awesome, great work!

@goodboy
Copy link
Contributor Author

goodboy commented May 18, 2018

Note I've also put up pytest-dev/pytest#3487 as requested to get this moving!

@fschulze
Copy link
Contributor

Could you add a hint to the deprecation warning on what to replace the functionality with?

@fschulze
Copy link
Contributor

Would it also be possible to throw a deprecation warning if a plugin provides a hook without using the marker? That is much more important, because people have to update their plugins first before the functionality is removed.

@fschulze
Copy link
Contributor

I would even say that there should be a pluggy release which only deprecates hooks without the marker before deprecating implprefix itself.

@goodboy
Copy link
Contributor Author

goodboy commented May 19, 2018

Could you add a hint to the deprecation warning on what to replace the functionality with?

Would it also be possible to throw a deprecation warning if a plugin provides a hook without using the marker?

@fschulze yes definitely to both of these.

I would even say that there should be a pluggy release which only deprecates hooks without the marker before deprecating implprefix itself.

@fschulze deprecating implprefix and the allowance of non-marked hookimpl functions is effectively the same thing (since there's no other way to collect hooks outside of implprefix scanning) so I'm not sure there's much point in 2 separate deprecation periods.

@fschulze
Copy link
Contributor

The difference is, that I would see DeprecationWarnings for implprefix as long as devpi supports plugins without the marker. What I would like is to let plugins see the warning that they should use the marker, but ignore the implprefix for that time. I guess I can use the warnings module to add a filter for it though.

@RonnyPfannschmidt
Copy link
Member

@fschulze see the related pytest pr for a solution

@goodboy
Copy link
Contributor Author

goodboy commented May 21, 2018

@fschulze pushed up the changes you requested. Let me know if that's good enough!

@fschulze
Copy link
Contributor

@tgoodlet Thanks! Now one knows what to look for in the documentation when this deprecation pops up.

Question about Travis:
Are the pluggy tests from this PR run at all? The envs checks, docs and benchmark don't look like the tests and the rest are tests against pytest.
Maybe this PR should make the deprecation warnings for implprefix into actual warnings in the tox.ini instead of errors when testing against pytest.

@goodboy
Copy link
Contributor Author

goodboy commented May 22, 2018

Question about Travis:
Are the pluggy tests from this PR run at all? The envs checks, docs and benchmark don't look like the tests and the rest are tests against pytest.

@fschulze Not sure what you mean. The tests are run with different versions of pytest (integration sanity checking) but the pluggy tests are what's run; check the CI job logs.

Maybe this PR should make the deprecation warnings for implprefix into actual warnings in the tox.ini instead of errors when testing against pytest.

I actually think the error on warnings is good because it shows where I haven't fixed up the tests yet. We should only be getting warnings where the tests are written to expect them.

@goodboy
Copy link
Contributor Author

goodboy commented May 22, 2018

Just waiting on CI after fixing up warning expectations in the tests then I'll merge as long as there's no more concerns 👍

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.

5 participants