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

Run mypy for tools/webdriver/ (but still allow untyped defs) #28913

Closed
wants to merge 2 commits into from

Conversation

foolip
Copy link
Member

@foolip foolip commented May 8, 2021

Part of #28833.

Changes are as minimal as possible. The annotation for Kwargs in
tools/wpt/utils.py was invalid syntax. `class Kwargs(Dict[Any, Any])`
would work but doesn't add any type information, so not done.

__init__.py files were added to various test directories so that they're
treated as packages, similar to tools/lint/tests/__init__.py and other
existing test directories in mypy-checked code.

Part of #28833.
Base automatically changed from foolip/mypy-tools to master May 11, 2021 08:30
jgraham
jgraham previously approved these changes May 11, 2021
Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

I guess

@@ -13,7 +13,7 @@
missing = object()


class ResponseHeaders(Mapping):
class ResponseHeaders(Mapping[AnyStr, str]):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this [AnyStr, str]? In particular I'm surprised at AnyStr since it seems like the keys should be consistently either strings or bytes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I should have left this as a draft, there are TODOs to figure out here still, note the "TODO: really?" I added.

I'll go ahead and close this and create a draft PR so you don't need to get notified as the sausage is made.

@jgraham jgraham dismissed their stale review May 11, 2021 08:47

Didn't mean to approve

@foolip foolip closed this May 11, 2021
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.

None yet

4 participants