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

restrict: loosen typing for list[str] to Iterable[str] and cast #230

Merged
merged 4 commits into from Nov 3, 2023

Conversation

ewjoachim
Copy link
Owner

@ewjoachim ewjoachim commented Oct 26, 2023

Closes #229 though I'd appreciate your feedback @miketheman :)

Checklist:

  • Tests
    • (not applicable?)
  • Documentation
    • (not applicable?)

@miketheman
Copy link

@ewjoachim Thanks for putting this together!

I think these changes would have masked the issue I was seeing from raising during a type-check.

Since the function signature now accepts an Iterable, that won't raise an issue when passing the a str.

For example, this python file named example.py

from typing import Iterable

def foo(names: Iterable[str]) -> list:
    return list(names)


def bar(names: list[str]) -> list:
    return list(names)


f1 = foo(names="example")
f2 = foo(names=["example"])

b1 = bar(names="example")
b2 = bar(names=["example"])

Running mypy:

$ mypy example.py
example.py:14: error: Argument "names" to "bar" has incompatible type "str"; expected "list[str]"  [arg-type]
Found 1 error in 1 file (checked 1 source file)

But since I was doing this in an interpreter, no mypy was run ;)

So I guess the "right" answer here would be to keep the types as is, and add the list() casting where applicable, since that would have avoided the raised exception.

If the function wants a list, let's keep that contract, and then add a check for it, and then we don't need to re-cast at all?

  if not isinstance(names, list):
    raise TypeError("Input must be a list")

Passing arguments of the wrong type (e.g. passing a list when an int is expected) should result in a TypeError, but passing arguments with the wrong value (e.g. a number outside expected boundaries) should result in a ValueError.
-- https://docs.python.org/3/library/exceptions.html#TypeError

@ewjoachim
Copy link
Owner Author

ewjoachim commented Oct 31, 2023

But then that would mean passing a generator or a set to project_names would be wrong, and I'm not sure it should be wrong (I'd rather it not to be wrong)

I think I'm trying to treat the issue so as to avoid embedding special cases in the code, trying to stay as open as possible for input types as long as I don't have to care what was passed and it works all the same for me.
At the same time, IIUC, you're not advocating for catching all types of errors on all args of all functions, but specifically catching strings instead of string iterables on the restrict function.
I guess it makes sense that from all possible user errors, it makes more sense to add safety bumps on the one at least one user has experienced. I'm wary of suddenly having to decide for all args of all functions if we need to add a runtime check or not, but it doesn't hurt adding a single one, and it makes sense that passing a single item in place of an iterable of items is probably a classic mistake.

@ewjoachim ewjoachim merged commit d80da1e into main Nov 3, 2023
6 of 7 checks passed
@ewjoachim ewjoachim deleted the restrict-iterable-str branch November 3, 2023 10:43
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.

Passing string to project_names results in broken experience
2 participants