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

Annotate types as optional #180

Closed
wants to merge 1 commit into from
Closed

Annotate types as optional #180

wants to merge 1 commit into from

Conversation

dsaxton
Copy link

@dsaxton dsaxton commented Jan 31, 2021

Summary

Hi all, typing nitpick here but I noticed there are a lot of optional types that are not annotated as such so I thought I'd start doing that. This doesn't cover them all, but if this is a change you would like to make I can fix some more (some function signatures are only partially fixed in this PR at the moment, for example).

Importance

Would allow for better inferences by tools like mypy.

Checklist

This PR:

  • adds new tests (if appropriate)
  • adds a change file in the changes/ directory (if appropriate)

@codecov-io
Copy link

Codecov Report

Merging #180 (de68cad) into master (01911f2) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@jlowin
Copy link
Member

jlowin commented Feb 1, 2021

Mypy automatically infers the Optional typing when a default is None, but I just learned that behavior is being deprecated: python/mypy#9091

@dsaxton
Copy link
Author

dsaxton commented Feb 1, 2021

Mypy automatically infers the Optional typing when a default is None, but I just learned that behavior is being deprecated: python/mypy#9091

Ah, I didn't know that, thanks! I'll close then, since this is a lot of extra boilerplate.

@dsaxton dsaxton closed this Feb 1, 2021
@dsaxton dsaxton deleted the optional-types branch February 1, 2021 03:19
@jlowin
Copy link
Member

jlowin commented Feb 1, 2021

Sorry @dsaxton, I didn't mean to write that to shut down the PR - I was going to share it with you in case you didn't know, but then learned it's now an unsupported behavior! I think it raises a good question of the degree to which we want to support mypy, as currently Server does not enforce mypy semantics (though Core does).

@dsaxton
Copy link
Author

dsaxton commented Feb 1, 2021

Sorry @dsaxton, I didn't mean to write that to shut down the PR - I was going to share it with you in case you didn't know, but then learned it's now an unsupported behavior! I think it raises a good question of the degree to which we want to support mypy, as currently Server does not enforce mypy semantics (though Core does).

No worries, let me know if you'd like to reopen. It does seem though that relying instead on the --implicit-optional flag may be the easier / preferred approach (depending on how pedantic we want to be about what the "correct" type is).

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.

3 participants