ewjoachim / pypitoken

Creation & manipulation of PyPI tokens
http://pypitoken.readthedocs.io/en/latest/
MIT License
12 stars 1 forks source link

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

Closed ewjoachim closed 11 months ago

ewjoachim commented 12 months ago

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

Checklist:

miketheman commented 12 months ago

@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 commented 12 months ago

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.