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

Avoid pkg_resources import at the top-level. #4768

Merged
merged 1 commit into from Mar 5, 2019

Conversation

Projects
None yet
5 participants
@horta
Copy link
Contributor

horta commented Feb 11, 2019

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs (you can delete this text from the final description, this is
just a guideline):

  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
  • Target the master branch for bug fixes, documentation updates and trivial changes.
  • Target the features branch for new features and removals/deprecations.
  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.

@horta horta force-pushed the horta:avoid-pkg_resources branch 2 times, most recently from 1ee15f3 to 749eddd Feb 11, 2019

@horta horta changed the title hide pkg_resources import Avoid pkg_resources import at the top-level. Feb 11, 2019

@horta horta force-pushed the horta:avoid-pkg_resources branch from 88ceb6b to 821b6ef Feb 11, 2019

@horta

This comment has been minimized.

Copy link
Contributor Author

horta commented Feb 11, 2019

Reduce from 0.3s to 0.1s import time on my machine.

@RonnyPfannschmidt
Copy link
Member

RonnyPfannschmidt left a comment

👍 nice jobs, this will make some things much more nice

i'd like to note that this currently just moves the cost as we currently do not use the entrypoints package to avoid it altogether, its just that pluggy also does lazy import of the lib

@@ -14,7 +14,6 @@

import py
import six
from pkg_resources import parse_version

This comment has been minimized.

@asottile

asottile Feb 12, 2019

Member

I'm -0

  • this won't actually help at all as the plugin system in the critical path imports setuptools / pkg_resources
  • I'd rather keep imports at the top

This comment has been minimized.

@nicoddemus

nicoddemus Feb 12, 2019

Member

Are there other alternatives for parsing versions that don't require pkg_resources?

I remember distutils has functionality for that, but @RonnyPfannschmidt was strictly against using it. Wouldn't make sense to use disutils just for this bit of functionality? I understand it should be avoided in general, but we are talking about a small utility function of something which is already in the stdlib, not about plugging into distutils in a big way or something.

I believe another point was that we require pkg_resources anyway because of the plugin system, but given pytest-dev/pluggy#190 it seems doable for us to move away from pkg_resources entirely.

This comment has been minimized.

@asottile

asottile Feb 12, 2019

Member

pkg_resources itself uses packaging (vendored)

This comment has been minimized.

@nicoddemus

nicoddemus Feb 12, 2019

Member

Interesting... perhaps we can consider adding it as dependency if that would mean we can drop pkg_resources from pytest and pluggy?

@blueyed blueyed merged commit 236bada into pytest-dev:master Mar 5, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build cancelled
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@blueyed

This comment has been minimized.

Copy link
Contributor

blueyed commented Mar 5, 2019

Merged, since it was approved, and #4768 (comment) could be done in a follow-up.

Thanks!

@asottile

This comment has been minimized.

Copy link
Member

asottile commented Mar 5, 2019

This doesn't actually improve anything (and obscures an import) so I was against merging it. @blueyed let's resolve the conversation next time before merging <3

@asottile

This comment has been minimized.

Copy link
Member

asottile commented Mar 5, 2019

Also it further conflicts my branch which rips all of this out

@blueyed

This comment has been minimized.

Copy link
Contributor

blueyed commented Mar 5, 2019

This doesn't actually improve anything (and obscures an import) so I was against merging it. @blueyed let's resolve the conversation next time before merging <3

You were -0.

Also it further conflicts my branch which rips all of this out

Sorry, but you did not state that you were working on this, and it should be trivial to resolve.

I was just going through the approved PRs, waiting for someone to approve #4887 - so that I could rebase branches on it.

@blueyed

This comment has been minimized.

Copy link
Contributor

blueyed commented Mar 5, 2019

@asottile
The next time please request changes.

@asottile

This comment has been minimized.

Copy link
Member

asottile commented Mar 5, 2019

@asottile
The next time please request changes.

I try to avoid "Request changes" because it produces a big angry red box

@blueyed

This comment has been minimized.

Copy link
Contributor

blueyed commented Mar 5, 2019

I try to avoid "Request changes" because it produces a big angry red box

Now you have a lila one ("Merged").. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.