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

Normalize to module imports #576

Merged
merged 6 commits into from
Feb 19, 2020
Merged

Conversation

bhrutledge
Copy link
Contributor

Per discussion in #570 and #572 (comment), and as suggested by the Google Style Guide:

Use import statements for packages and modules only, not for individual classes or functions

Additional changes:

  • Renamed frequently-used package variable to pkg, because this change introduced more instances of from twine import package; I saw pkg used elsewhere in the code

  • Used higher-level APIs (e.g. requests.HTTPError instead of requests.exceptions.HTTPError, urllib3 instead of requests.packages.urllib3, etc.) when I saw the opportunity

  • Removed the conditional checks for hashlib.blake2b, since that's included in Python 3.6+

  • Left imports from contextlib and urllib.parse alone, because those seem to be accepted conventions.

If folks are curious, here's the current results for from .*import: from_import.txt.

@sigmavirus24
Copy link
Member

Moving to three-letter unprouncable variable names is actively hostile towards other developers who have to use screen readers. This talk discusses it in the context of Go but it's equally as applicable to Python codebases.

twine/repository.py Outdated Show resolved Hide resolved
@bhrutledge
Copy link
Contributor Author

I can see this was overzealous and posisibly premature (though I don't consider it wasted effort, because it further familiarized me with the codebase).

Moving to three-letter unprouncable variable names is actively hostile

Not my desire at all. Thanks for the link to the talk; will watch. For what it's worth, I picked it up from test_package.py

changing the naming of an argument is a backwards-incompatible change

🤦‍♂. Any suggestions on how to keep package but avoid from twine.package import PackageFile? Maybe from twine import package as pkg?

@sigmavirus24
Copy link
Member

I can see this was overzealous and posisibly premature

I don't think it was.

Any suggestions on how to keep package but avoid from twine.package import PackageFile? Maybe from twine import package as pkg?

The sole purpose of that file is to use PackageFile so maybe we should from twine import package as package_file?

@bhrutledge
Copy link
Contributor Author

I can see this was overzealous and posisibly premature

I don't think it was.

Thanks. 😅

maybe we should from twine import package as package_file?

Done.

Having isort and black in place made this task easier, though it raised a question about how to deal with as imports: should they go on their own line? You can see the difference between the two modes in f91fe38. I don't feel strongly.

@sigmavirus24
Copy link
Member

I find the separate lines version easier to read personally

@bhrutledge
Copy link
Contributor Author

I find the separate lines version easier to read personally

Done.

@bhrutledge
Copy link
Contributor Author

@pypa/twine-maintainers Any objections to merging this? I'd like to do it this week.

Looks like the failing coverage check might be related to removing the conditional around blake2b.

tests/test_main.py Outdated Show resolved Hide resolved
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.

2 participants