-
Notifications
You must be signed in to change notification settings - Fork 17
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
fix: add mypy checking + 'py.typed' files #149
Conversation
FBO typing, which interprets 'py.typed' file as always-inherited in subdirs.
setup.py
Outdated
@@ -39,7 +39,7 @@ | |||
package_root = os.path.abspath(os.path.dirname(__file__)) | |||
|
|||
version = {} | |||
with open(os.path.join(package_root, "google/cloud/version.py")) as fp: | |||
with open(os.path.join(package_root, "google/cloud/version/__init__.py")) as fp: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might require an update to https://github.com/googleapis/release-please/blob/main/src/releasers/python.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than that, maybe we just back out converting google/cloud/version.py
to a package? I don't really see anybody choking on lack of typing when importing the one attribute in that module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backed out in c77b9dc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. It's literally just 1 string, so I'm okay with that.
Our 'release-please' tooling won't find it there, and nobody is likely to a) import the module, and then b) choke on its lack of typing.
Confirmed that it's passing on Kokoro:
|
This should go in a minor release, not a patch release. I probably should have marked as |
No description provided.