Conversation
distutils/command/build_py.py
Outdated
|
|
||
| def build_packages(self) -> None: | ||
| for package in self.packages: | ||
| for package in self.packages or (): |
There was a problem hiding this comment.
This is a change in behaviour as it would graciously do nothing instead of raising if self.packages is still None.
If it makes more sense to raise, a clearer error than TypeError: 'NoneType' object is not iterable can be raised instead.
There was a problem hiding this comment.
Presumably self.packages is expected not to be None at this point. I'd rather see it enforced that it's not None by the time it reaches this point. That's probably more work than it's worth, so for a local fix, let's just do an explicit check (if self.packages is None raise a clear error).
There was a problem hiding this comment.
Feel free to update the new message to your likings !
| _wordchars_re = re.compile(rf'[^\\\'\"{string.whitespace} ]*') | ||
| _squote_re = re.compile(r"'(?:[^'\\]|\\.)*'") | ||
| _dquote_re = re.compile(r'"(?:[^"\\]|\\.)*"') |
There was a problem hiding this comment.
This isn't exactly "heavy computing" is it? Did it need to be done lazily ?
There was a problem hiding this comment.
Agreed. Looks like premature optimization. Even the re.compile calls are probably premature. Probably could have just used simple strings and compiled on demand and relied on caching in the re module to avoid duplicate work, but this approach is clean and simple enough.
| _wordchars_re = re.compile(rf'[^\\\'\"{string.whitespace} ]*') | ||
| _squote_re = re.compile(r"'(?:[^'\\]|\\.)*'") | ||
| _dquote_re = re.compile(r'"(?:[^"\\]|\\.)*"') |
There was a problem hiding this comment.
Agreed. Looks like premature optimization. Even the re.compile calls are probably premature. Probably could have just used simple strings and compiled on demand and relied on caching in the re module to avoid duplicate work, but this approach is clean and simple enough.
distutils/command/build_py.py
Outdated
|
|
||
| def build_packages(self) -> None: | ||
| for package in self.packages: | ||
| for package in self.packages or (): |
There was a problem hiding this comment.
Presumably self.packages is expected not to be None at this point. I'd rather see it enforced that it's not None by the time it reaches this point. That's probably more work than it's worth, so for a local fix, let's just do an explicit check (if self.packages is None raise a clear error).
| assert isinstance(cc, str) | ||
| assert isinstance(cxx, str) | ||
| assert isinstance(cflags, str) | ||
| assert isinstance(ccshared, str) | ||
| assert isinstance(ldshared, str) | ||
| assert isinstance(ldcxxshared, str) | ||
| assert isinstance(shlib_suffix, str) | ||
| assert isinstance(ar_flags, str) | ||
| ar = os.environ.get('AR', ar) | ||
| assert isinstance(ar, str) |
There was a problem hiding this comment.
I mentioned in another PR - I'm not sure these changes are stable. I'm worried there are edge cases that would begin to fail with this change. Let's be confident that this change doesn't break anything (and consider using casts to retain the current behavior if appropriate).
Avasam
force-pushed
the
mypy--Fix-all-union-attr
branch
from
1d367ee to
9876b34
Compare
2 participants
No description provided.