Colorize daemon output and add summary line #7441
Conversation
mypy/dmypy/client.py
Outdated
| @@ -265,12 +265,14 @@ def do_run(args: argparse.Namespace) -> None: | |||
| # Bad or missing status file or dead process; good to start. | |||
| start_server(args, allow_sources=True) | |||
| t0 = time.time() | |||
| response = request(args.status_file, 'run', version=__version__, args=args.flags) | |||
| response = request(args.status_file, 'run', version=__version__, args=args.flags, | |||
| no_tty=not sys.stdout.isatty()) | |||
There was a problem hiding this comment.
The way no_tty argument is used seems error-prone, since the caller accepts **kwargs: object. Why not calculate it in request unless explicitly provided. Maybe just send it always to server, for consistency? The server can then decide if it wants to deal with it. This would reduce some coupling, as if some command starts producing colored output, only the server would have to be updated. Anyway, at least it would be good to explicitly declare the argument in request if you don't get rid of it.
Also style nit: I'd avoid an argument with a negated boolean value. This could be renamed to is_tty, for example, and it would arguably be cleaner.
There was a problem hiding this comment.
Why not calculate it in
requestunless explicitly provided. Maybe just send it always to server, for consistency?
Yes, I was actually also not happy that no_tty in **args would not be statically checked. I will try to tighten this.
Also style nit: I'd avoid an argument with a negated boolean value. This could be renamed to
is_tty, for example, and it would arguably be cleaner.
OK, makes sense (also for the no_color below).
mypy/dmypy/client.py
Outdated
| # If the daemon signals that a restart is necessary, do it | ||
| if 'restart' in response: | ||
| print('Restarting: {}'.format(response['restart'])) | ||
| restart_server(args, allow_sources=True) | ||
| response = request(args.status_file, 'run', version=__version__, args=args.flags) | ||
| response = request(args.status_file, 'run', version=__version__, args=args.flags, | ||
| no_tty=not sys.stdout.isatty()) |
There was a problem hiding this comment.
The above suggestion would get rid of the repeated not sys.stdout.isatty() calls.
mypy/util.py
Outdated
| return self.style('Success: no issues found in {}' | ||
| ' source file{}'.format(n_sources, 's' if n_sources != 1 else ''), | ||
| 'green', bold=True) | ||
| def format_success(self, n_sources: int, no_color: bool = False) -> str: |
There was a problem hiding this comment.
Similar to above, I'd prefer to avoid no_xxx boolean variable names.
mypy/dmypy_server.py
Outdated
| not self.options.color_output or no_tty) | ||
| else: | ||
| summary = self.formatter.format_success(n_sources, | ||
| not self.options.color_output or no_tty) |
There was a problem hiding this comment.
The not self.options.color_output or no_tty expression is repeated here. It would be less error-prone to calculate it only once.
mypy/dmypy_server.py
Outdated
| if summary: | ||
| # Create new list to avoid appending multiple summaries on successive runs. | ||
| messages = messages + [summary] | ||
| if self.options.color_output and not no_tty: |
There was a problem hiding this comment.
Here is the third repeat of this expression.
|
@msullivan I also added support for |
|
Since the review comments were all minor here, I am going to merge this when tests pass (unless there are new comments or objections) and then rebase and update #7440 on top of this. |
Fixes #7438
This PR makes the daemon behave the same way as the normal run (i.e. use colorized output and write a summary line). I also fix a minor bug that the summary line was colorized even if
--no-color-outputwas passed.The idea is quite simple, client passes to the server info about whether we are in tty and if yes server returns nicely formatted output. I tried running various commands on Linux and piping it through
grep.