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
Merged

Conversation

@goodboy
Copy link
Member

@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

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']

This comment has been minimized.

@nicoddemus

nicoddemus Aug 24, 2017
Member

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``

This comment has been minimized.

@nicoddemus

nicoddemus Aug 24, 2017
Member

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

This comment has been minimized.

@nicoddemus

nicoddemus Aug 24, 2017
Member

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

This comment has been minimized.

@goodboy

goodboy Aug 25, 2017
Author Member

@nicoddemus yep I'll test it out

This comment has been minimized.

@goodboy

goodboy Aug 25, 2017
Author Member

seems to have worked 👍

from pluggy import * # noqa
from pluggy import __version__ # noqa
from pluggy import *
from pluggy import __version__

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Aug 25, 2017
Member

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

This comment has been minimized.

@nicoddemus

nicoddemus Aug 25, 2017
Member

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

This comment has been minimized.

@goodboy

goodboy Aug 25, 2017
Author Member

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

This comment has been minimized.

@nicoddemus

nicoddemus Aug 25, 2017
Member

Awesome, thanks!

@goodboy
Copy link
Member Author

@goodboy 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

@RonnyPfannschmidt RonnyPfannschmidt commented Aug 25, 2017

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

Copy link
Member

@nicoddemus nicoddemus left a comment

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
Member Author

@goodboy 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

@nicoddemus nicoddemus commented Aug 25, 2017

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
Member Author

@goodboy goodboy commented Aug 25, 2017

@nicoddemus good enough?

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Aug 25, 2017

Yep, thanks!

@goodboy
Copy link
Member Author

@goodboy 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

@The-Compiler The-Compiler commented Aug 28, 2017

That seems odd - I opened HypothesisWorks/hypothesis#822

@goodboy
Copy link
Member Author

@goodboy goodboy commented Aug 28, 2017

Thanks @The-Compiler :)

@goodboy
Copy link
Member Author

@goodboy 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

@RonnyPfannschmidt RonnyPfannschmidt commented Aug 29, 2017

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
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@goodboy
Copy link
Member Author

@goodboy 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants