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

#11655 Remove use of pkg_resources #11730

Merged
merged 7 commits into from Oct 28, 2022
Merged

#11655 Remove use of pkg_resources #11730

merged 7 commits into from Oct 28, 2022

Conversation

ofek
Copy link
Contributor

@ofek ofek commented Oct 26, 2022

Scope and purpose

Fixes #11655

See #11728 (comment)

@ofek
Copy link
Contributor Author

ofek commented Oct 26, 2022

Copy link
Member

@glyph glyph left a comment

Choose a reason for hiding this comment

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

Looks fine, but I'm not sure it's going to pass coverage. I'll switch the merge base to trunk now that it's reviewed, and hopefully that will run through CI

src/twisted/trial/__main__.py Outdated Show resolved Hide resolved
@glyph glyph changed the title Remove use of pkg_resources #11655 Remove use of pkg_resources Oct 26, 2022
@glyph glyph changed the base branch from revert-11727-revert-11656-modernize-metadata to trunk October 26, 2022 16:37
glyph and others added 2 commits October 26, 2022 09:39
Co-authored-by: Glyph <glyph@twistedmatrix.com>
@@ -4,6 +4,15 @@
if __name__ == "__main__":
import sys

from pkg_resources import load_entry_point
if sys.version_info >= (3, 8):
Copy link
Member

Choose a reason for hiding this comment

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

Sigh. Okay. Codecov is too smart for our trickery.

Which actually raises the question: why are we doing this faffing with packaging metadata, anyway? We used to do this because we wanted to be as similar as possible to the stubs that pip install generated, but looking at a modern virtualenv, I see the stubs look like this:

#!/Users/glyph/.virtualenvs/Twisted/bin/python
# -*- coding: utf-8 -*-
import re
import sys
from twisted.scripts.trial import run
if __name__ == '__main__':
    sys.argv[0] = re.sub(r'(-script\.pyw|\.exe)?$', '', sys.argv[0])
    sys.exit(run())

So that implies that this should instead simply be:

    from twisted.scripts.trial import run
    sys.exit(run())

Is there any reason not to do that?

(Suggestion in multiple parts below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very true

Comment on lines 7 to 10
if sys.version_info >= (3, 8):
from importlib.metadata import distribution
else:
from importlib_metadata import distribution
Copy link
Member

Choose a reason for hiding this comment

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

Part 1: no need for this package at all

Suggested change
if sys.version_info >= (3, 8):
from importlib.metadata import distribution
else:
from importlib_metadata import distribution

Comment on lines 12 to 18
sys.exit(
next(
ep
for ep in distribution("twisted").entry_points
if (ep.group, ep.name) == ("console_scripts", "trial")
).load()()
)
Copy link
Member

Choose a reason for hiding this comment

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

Part 2: let's not overcomplicate things

Suggested change
sys.exit(
next(
ep
for ep in distribution("twisted").entry_points
if (ep.group, ep.name) == ("console_scripts", "trial")
).load()()
)
from twisted.scripts.trial import run
sys.exit(run())

Copy link
Member

@glyph glyph left a comment

Choose a reason for hiding this comment

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

Oops. Whatever we do here, twisted/__main__.py also needs to be fixed in the same way.

Copy link
Member

@glyph glyph left a comment

Choose a reason for hiding this comment

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

Made a suggestion to keep the script import inside the if __name__ block, but otherwise, if this passes tests let's land.

src/twisted/__main__.py Outdated Show resolved Hide resolved
src/twisted/__main__.py Show resolved Hide resolved
ofek and others added 2 commits October 26, 2022 14:32
Co-Authored-By: Glyph <glyph@twistedmatrix.com>
@ofek
Copy link
Contributor Author

ofek commented Oct 28, 2022

rebased

@adiroiban adiroiban merged commit 9e92c8b into twisted:trunk Oct 28, 2022
@adiroiban
Copy link
Member

Many thanks @ofek for the fix and @glyph for the review.

I have merged this on behalf of @glyph :)

@ofek ofek deleted the fix branch October 28, 2022 13:51
@glyph
Copy link
Member

glyph commented Oct 29, 2022

Thanks @adiroiban !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update package metadata
4 participants