Accept any non-string iterable for distutils.Extension's sources
#311
Conversation
|
I have a bunch of failing tests, linter errors, and several warnings from VS Code extensions locally; I hope the CI will be cleaner in its output. :) |
agriyakhetarpal
force-pushed
the
types/extension-sources
branch
from
e7aba2d to
115bb67
Compare
distutils/extension.py
Outdated
| raise AssertionError("'name' must be a string") # noqa: TRY004 | ||
| if not ( | ||
| isinstance(sources, list) | ||
| and all(isinstance(v, (str, os.PathLike)) for v in sources) | ||
| ): | ||
|
|
||
| # we handle the string case first; though strings are iterable, we disallow them | ||
| if isinstance(sources, str): | ||
| raise AssertionError( # noqa: TRY004 | ||
| "'sources' must be an iterable of strings or PathLike objects, not a string" | ||
| ) | ||
|
|
||
| # mow we check if it's iterable and contains valid types | ||
| try: | ||
| sources = list(sources) # convert to list for consistency | ||
| if not all(isinstance(v, (str, os.PathLike)) for v in sources): | ||
| raise AssertionError( | ||
| "All elements in 'sources' must be strings or PathLike objects" | ||
| ) | ||
| except TypeError: | ||
| raise AssertionError( |
There was a problem hiding this comment.
Question for the distutils maintainers: I can't help but feel that AssertionError is misused here, shouldn't these all be TypeError ? (it's noqa'd but not explained)
There was a problem hiding this comment.
I have the same suggestion! :)
There was a problem hiding this comment.
Peeking at the history, these used to be actual assert statements (7d70ca7), so my guess is that the AssertionErrors were kept for backwards compatibility with code that might try to catch these errors.
There was a problem hiding this comment.
Additionally, what I infer from the commit message is that that they were added because in-line assert statements are completely removed from the bytecode if used with -O
There was a problem hiding this comment.
I suspect any Exception here is fine, so a TypeError should be fine (famous last words).
There was a problem hiding this comment.
For uniformity, I changed the AssertionError for both the name and the sources checks in efeb97c to be a TypeError.
Co-Authored-By: Avasam <samuel.06@hotmail.com>
|
To expand on the additional context: This allows typing the |
Co-authored-by: Jason R. Coombs <jaraco@jaraco.com> Co-authored-by: Avasam <samuel.06@hotmail.com>
| sources : Iterable[string | os.PathLike] | ||
| iterable of source filenames (except strings, which could be misinterpreted | ||
| as a single filename), relative to the distribution root (where the setup | ||
| script lives), in Unix form (slash-separated) for portability. Can be any | ||
| non-string iterable (list, tuple, set, etc.) containing strings or | ||
| PathLike objects. Source files may be C, C++, SWIG (.i), platform-specific | ||
| resource files, or whatever else is recognized by the "build_ext" command | ||
| as source for a Python extension. |
There was a problem hiding this comment.
I just realized a possible inaccuracy: This documents instance attributes, not parameters. In which case the old docstring was (almost) correct. (sources is still a list[str], not a list[string | os.PathLike] because of os.fspath)
Description
This PR continues what was discussed in python/typeshed#12958 (comment) on allowing a loosened check for the source files for a
distutilsExtensioninstance.Previously: just lists were accepted; now: any non-string iterable is accepted (tuples, sets, other iterators, etc.). I plan to update python/typeshed in accordance with this change.
Additional context
xref: python/mypy#18107, python/typeshed#12958