-
Notifications
You must be signed in to change notification settings - Fork 84
Widen return type of CCompiler.runtime_library_dir_option().
#339
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
Conversation
Subclasses of `CCompiler`, specifically those in `unix` and `cygwin`, can return lists of strings in addition to strings. Whether this should be loosened further, e.g. `str | Sequence[str]`, is up for debate, but at least adding `list[str]` makes this more accurate.
CCompiler.runtime_library_dir_option().CCompiler.runtime_library_dir_option().
|
This change is correct, and mypy currently even catches it in my editor, wonder why not on CI... Union return types have a bit of a gotcha, but I think in this case it'll be fine (both
|
|
Fair, Union return types aren't the greatest, though I think it's unavoidable in this case since it's the reality of the return type in practice. Maybe that can be changed in the future, but that's probably out of the scope of this PR. |
|
As for why this isn't caught in CI, well, it looks like mypy isn't run in CI. tox is installed and run, and tox in turn installs pytest-mypy, but pytest-mypy doesn't do anything because If I test this locally with Output(EDIT: Replaced output with output when run on the correct branch. Whoops.) This probably deserves its own issue. |
In theory the tests use |
|
Ah, gotcha. Glancing at the config, pytest-enabler is intentionally off because "distutils isn't ready yet". Guess that answers that. Lines 86 to 88 in b53777a
|
|
Mypy tests have been re-enabled in #349 |
As expected, since you fixed the underlying issue. You can just remove that type-ignore |
|
This should be good to go now. |
|
@abravalheri tests have been re-enabled and this PR passes. |

Existing subclasses of
CCompileroverride this method to return a string or a list of strings. For example, those forcygwinandunix(see below).distutils/distutils/compilers/C/cygwin.py
Lines 218 to 223 in b53777a
distutils/distutils/compilers/C/unix.py
Line 312 in b53777a
Altering the return annotation of the base class's method should make it more compatible and accurate with respect to those subclasses (and possibly other, third-party ones).