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

Add error-code for truthy-iterable #13762

Merged
merged 3 commits into from
Nov 13, 2022
Merged

Conversation

cdce8p
Copy link
Collaborator

@cdce8p cdce8p commented Sep 29, 2022

Add a separate error code for truthy-iterable.

Iterable does not implement __len__, so passing a Generator would always evaluate to true. Suggest Collection as alternative.

In most cases this isn't an issue as the function is often called with list anyway. However, the dedicated check can pinpoint subtle errors that might get unnoticed otherwise.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@cdce8p cdce8p marked this pull request as ready for review September 29, 2022 03:55
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@cdce8p
Copy link
Collaborator Author

cdce8p commented Sep 29, 2022

@hauntsaninja This is related to #13686, this time for Iterable though. I think there is value in having a separate error code for it. Whether it should be enabled by default is a different question. Just for Home Assistant I'm going to adjust the typing for all but one instance. In practice none of them is a true issue though, just a possible one mypy wouldn't have noticed.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@cdce8p
Copy link
Collaborator Author

cdce8p commented Nov 9, 2022

Pushed a new commit to disable the error-code by default. That should minimize the impact of the PR.

@cdce8p
Copy link
Collaborator Author

cdce8p commented Nov 13, 2022

@JelleZijlstra Would you mind taking a look? The actual code change is quite small but I think it will be an improvement never the less. The PR is ready from my POV.

docs/source/error_code_list2.rst Outdated Show resolved Hide resolved
@@ -153,6 +153,10 @@ def with_additional_msg(self, info: str) -> ErrorMessage:
FUNCTION_ALWAYS_TRUE: Final = ErrorMessage(
"Function {} could always be true in boolean context", code=codes.TRUTHY_FUNCTION
)
ITERABLE_ALWAYS_TRUE: Final = ErrorMessage(
"{} which can always be true in boolean context. Consider using {} instead.",
Copy link
Member

Choose a reason for hiding this comment

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

This message reads a bit weird to me but I guess it's consistent with the other ALWAYS_TRUE messages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On its own, I agree. It gets better once the variables are inserted. E.g.

"var" has type "Iterable[str]" which can always be true in boolean context.
Consider using "Collection[str]" instead.  [truthy-iterable]

Comment on lines 234 to 236
This is similar in concept to ensuring that an expression's type implements an expected interface (e.g. ``Sized``),
except that attempting to invoke an undefined method (e.g. ``__len__``) results in an error,
while attempting to evaluate an object in boolean context without a concrete implementation results in a truthy value.
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what this sentence is trying to say. What is "this" exactly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As best as I can tell, "this" refers to the check itself and the fact that it doesn't highlight a concrete issue but rather only a potential one.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra JelleZijlstra merged commit 33d2b89 into python:master Nov 13, 2022
@cdce8p cdce8p deleted the truthy-iterable branch November 13, 2022 20:59
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.

None yet

2 participants