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

Stop vendoring pluggy #2719

Merged
merged 7 commits into from
Aug 29, 2017

Conversation

goodboy
Copy link

@goodboy goodboy commented Aug 24, 2017

If this is too much let me know guys ;)
Pretty sure I got everything but someone should double check.

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.

Thanks a lot @tgoodlet!

setup.py Outdated
@@ -43,7 +43,7 @@ def has_environment_marker_support():


def main():
install_requires = ['py>=1.4.33', 'six>=1.10.0','setuptools'] # pluggy is vendored in _pytest.vendored_packages
install_requires = ['py>=1.4.33', 'six>=1.10.0','setuptools', 'pluggy==0.4.0']
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps 'pluggy>=0.4.0,<0.5'?

@@ -1,11 +1,6 @@
"""
imports symbols from vendored "pluggy" if available, otherwise
falls back to importing "pluggy" from the default namespace.
Import symbols from ``pluggy``
Copy link
Member

Choose a reason for hiding this comment

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

I think we can get rid of this module altogether and just import pluggy docs normally no?

@@ -454,7 +454,7 @@ hook wrappers and passes the same arguments as to the regular hooks.

At the yield point of the hook wrapper pytest will execute the next hook
implementations and return their result to the yield point in the form of
a :py:class:`CallOutcome <_pytest.vendored_packages.pluggy._CallOutcome>` instance which encapsulates a result or
a :py:class:`CallOutcome <_pytest._pluggy._CallOutcome>` instance which encapsulates a result or
Copy link
Member

Choose a reason for hiding this comment

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

Could you please create the docs locally (tox -e docs) to ensure the changes links still work?

Copy link
Author

Choose a reason for hiding this comment

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

@nicoddemus yep I'll test it out

Copy link
Author

Choose a reason for hiding this comment

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

seems to have worked 👍

from pluggy import * # noqa
from pluggy import __version__ # noqa
from pluggy import *
from pluggy import __version__
Copy link
Member

Choose a reason for hiding this comment

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

we might want to consider just directly inlining the imports as a followup

Copy link
Member

Choose a reason for hiding this comment

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

I think we can do it in the same PR if @tgoodlet is willing

Copy link
Author

Choose a reason for hiding this comment

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

@nicoddemus @RonnyPfannschmidt yup no problem guys.
I'll take a look this morning.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks!

@goodboy
Copy link
Author

goodboy commented Aug 25, 2017

@RonnyPfannschmidt @nicoddemus alright made the changes.
If you guys want me to tighten up the history a bit let me know.

@RonnyPfannschmidt
Copy link
Member

personally i like the history intact, but please add a changelog fragment

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.

Thanks, looks good!

Just missing adding a trivial changelog entry like @RonnyPfannschmidt asked and then is good to merge from my POV.

Regarding history, I like to tidy the history (and always do on my PRs), so we can leave that to you. 😁

@goodboy
Copy link
Author

goodboy commented Aug 25, 2017

@nicoddemus @RonnyPfannschmidt I noticed in the pip docs they have

Upgrading, removing, or adding a new vendored library gets a special mention using a news/.vendor file. This is in addition to any features, bugfixes, or other kinds of news that pulling in this library may have. This uses the library name as the key so that updating the same library twice doesn’t produce two news file entries.

Are we doing that as well or do I just need to add a .trivial entry to the changelog/ dir?

@nicoddemus
Copy link
Member

IMO just the trivial entry; pluggy is such an internal part of pytest right now that most users are not even aware of it or its bug-fixes.

@goodboy
Copy link
Author

goodboy commented Aug 25, 2017

@nicoddemus good enough?

@nicoddemus
Copy link
Member

Yep, thanks!

@goodboy
Copy link
Author

goodboy commented Aug 28, 2017

@nicoddemus I'm assuming those CI failures are expected?
I noticed some odd (hypothesis) failures when running tox locally.

@The-Compiler
Copy link
Member

That seems odd - I opened HypothesisWorks/hypothesis#822

@goodboy
Copy link
Author

goodboy commented Aug 28, 2017

Thanks @The-Compiler :)

@goodboy
Copy link
Author

goodboy commented Aug 29, 2017

Once you guys merge this I'll put up a new PR to bring in pluggy 0.5.0 which should give us speed improvements as we deprecate __multicall__ support 😄

@RonnyPfannschmidt
Copy link
Member

another item to consider, we could now cython-ize pluggy for cpython ^^
argument extraction can be sped up quite a bit

@RonnyPfannschmidt RonnyPfannschmidt merged commit 488bbd2 into pytest-dev:features Aug 29, 2017
@goodboy
Copy link
Author

goodboy commented Aug 29, 2017

@RonnyPfannschmidt good idea.
I'd love to take a stab at that.

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.

4 participants