[refactor] increase consistency in stdout and stderr optional arguments #19638
Conversation
This comment has been minimized.
This comment has been minimized.
|
I still think there's a good change to be had in here, but I'm trying a different approach. So, it stays as a draft PR for now. original comment, with the approach " [refactor] simply use default arguments for stdout and stderr"Unless this runs me into the famous Python default argument folly, which doesn't seem likely to me, then this should be a better way to do it. (What, do we change sys.stdout during the course of the program? Guess CI will let me find out...) I guess you could argue that even trying to access a member variable like this sets us up for the footgun in the future... Edit: "you could argue" — I've been convinced by my own argument. Also, I guess some of our test code, like a tiny amount, one test it seems, does actually rely on retargetting stderr or something. |
wyattscarpenter
force-pushed
the
stdout
branch
from
7333e1b to
cf0de50
Compare
for more information, see https://pre-commit.ci
wyattscarpenter
changed the title
for more information, see https://pre-commit.ci
| @@ -2016,7 +2016,7 @@ def test_stubs(args: _Arguments, use_builtins_fixtures: bool = False) -> int: | |||
| def set_strict_flags() -> None: # not needed yet | |||
| return | |||
|
|
|||
| parse_config_file(options, set_strict_flags, options.config_file, sys.stdout, sys.stderr) | |||
| parse_config_file(options, set_strict_flags, options.config_file, sys.stderr) | |||
There was a problem hiding this comment.
Why does this one directly use sys.stderr? I don't know, probably some reason. It was like that when I got here.
| (f_out is None or f_err is None) | ||
| or not should_force_color() | ||
| and (not f_out.isatty() or not f_err.isatty()) | ||
| ): | ||
| self.dummy_term = True |
There was a problem hiding this comment.
I'm not sure whether it matters whether or not it's a dummy term if stdout or stderr are None, but since None means no printing I figured that's a pretty dummy term... (In case it's not clear: the only reason I included the check for None is because python has no safe navigation operator that would let us go f_out.isatty())
There was a problem hiding this comment.
The check could also be distributed like
if not should_force_color() and (not (f_out and f_out.isatty()) or not (f_err and f_err.isatty()))I have no strong feelings on that.
This comment has been minimized.
This comment has been minimized.
I guess reasonable minds could disagree on this
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
I have made the handling of optional stdout and stderr arguments more consistent — which inconsistent situation I was partially responsible for. I spent a while thinking about the right way to handle these. Because sys.stdout can be retargeted, and because of the famous weird way python handles default values, it's probably not good to do it the simplest possible way, which would invite confusion later. Instead, we do it the second-simplest way possible, with sentinel values. This necessitates changing some other type signatures.
On another note, sys.stdout and sys.stderr can themselves technically be None, and typeshed gives them MaybeNone to pay homage to this — I would like to actually handle this correctly in the mypy code here, but, what are you going to do, write a message to stderr about it? Guess we'll just roll on. Print automatically handles TextIO|None, anyway example from stackoverflow.
One of my main motivations for making this PR is the fact that pyright doesn't narrow eg
stdout or sys.stdoutto TextIO. However, that's not really Mypy's problem, I guess. Although it is a bonus that mypy now handles the actual type of sys.stdout, which can be None (although the way it handles it, now, is a bit odd, since it tries to reattach to sys.stdout in several places). But, I felt that, once I had refactored several things, it was worth the PR.