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

Use Literal for statistics.quantiles() #7463

Merged
merged 3 commits into from
Mar 8, 2022
Merged

Use Literal for statistics.quantiles() #7463

merged 3 commits into from
Mar 8, 2022

Conversation

Akuli
Copy link
Collaborator

@Akuli Akuli commented Mar 8, 2022

Fixes #7462

@github-actions

This comment has been minimized.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Looks good. I don't think #7258 needs to block this.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 2022

Diff from mypy_primer, showing the effect of this PR on open source code:

core (https://github.com/home-assistant/core)
+ homeassistant/components/statistics/sensor.py:646: error: Argument "method" to "quantiles" has incompatible type "str"; expected "Union[Literal['inclusive'], Literal['exclusive']]"  [arg-type]

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Mar 8, 2022

Given that there are false positives in practice, "prefer false negatives over false positives" would imply we shouldn't do this without a fallback overload right?

@AlexWaygood
Copy link
Member

Given that there are false positives in practice, "prefer false negatives over false positives" would imply we shouldn't do this without a fallback overload right?

How would we do a fallback overload in this case?

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

I think this is a good change to make. The one false positive is very easily fixed. #7258 is a temporary moratorium on literal constants, not on literal argument types.

@hauntsaninja
Copy link
Collaborator

It'd be a bad fallback in that we'd have zero additional type safety but would still mean we get better IDE hover docs / completion docs :-)
I don't feel too strongly, so feel free to merge, but there is still a similar consideration on "soft literal"s here. The ideal type checker behaviour for me here is "if we know the value of the string, make sure it matches the literals. if we don't know, allow it", and we don't have a way of spelling that currently.

@AlexWaygood AlexWaygood merged commit f9cb7c3 into master Mar 8, 2022
@AlexWaygood AlexWaygood deleted the Akuli-patch-1 branch March 8, 2022 21:38
@AlexWaygood
Copy link
Member

Shoot, I screwed up the commit description a bit there. Apologies :)

@Akuli
Copy link
Collaborator Author

Akuli commented Mar 8, 2022

It happens :) It's not super critical with stdlib stubs anyway. With third-party stubs, this matters more because commit messages turn into an auto-generated changelog.

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.

Use Literal for statistics.quantiles()
4 participants