Much improved output formatting #45
Conversation
Chroxvi
changed the title
| assert stdout == stdout_spin_and_final_line | ||
| assert stderr == stderr_spin_and_final_line | ||
|
|
||
| @pytest.mark.parametrize("spins", [1]) |
There was a problem hiding this comment.
I found the spins variable slightly confusing when looking at the code. It took me a little bit to figure out that it was an argument to patch_fix_number_of_message_spins. There is a comment about it in the patch file, but I wonder if there should be one where it is used as well.
There was a problem hiding this comment.
I have added comments for all test cases that make use of this "override a fixture with direct test parametrization"-approach to passing a variable from the test case to the fixture.
See https://stackoverflow.com/questions/18011902/how-to-pass-a-parameter-to-a-fixture-function-in-pytest for a discussion of alternative ways to achieve the same result.
| "increase the verbosity of the output from cotainr. " | ||
| "Can be used multiple times: Once for subprocess output, " | ||
| "twice for subprocess INFO, three times for DEBUG, " | ||
| "and four times for TRACE" |
There was a problem hiding this comment.
maybe add an example like -v -v e.g. for INFO. is a bit hard to understand. Not a blocker.
There was a problem hiding this comment.
Good idea. I have added it to the backlog.
| from . import util | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
may be a matter of style and different in python but should it not be ok just to have the static logger and alway call on that and not create a copy/ref here? - Not a blocker.
There was a problem hiding this comment.
This actually does provide a static (hierarchy of) logger(s). The logging module keeps track of all loggers added via logging.getLogger and returns a reference to a logger if it has already been created (which is also why we need to reload the logging modules in our tests, to reset this internal state). The pattern logger = logging.getLogger(__name__) is just the recommended way to maintain a hierarchy of loggers in line with the package/module hierarchy (see https://docs.python.org/3/library/logging.html#logger-objects for details).
| # When the command to be run in the container provides its own | ||
| # log_dispatcher | ||
| with custom_log_dispatcher.prefix_stderr_name( | ||
| prefix=self.__class__.__name__ |
There was a problem hiding this comment.
for a future change should we create better names for the outb`put than use the class name?
There was a problem hiding this comment.
Sounds reasonable. I wasn't able to come up with better short names than the class names, though.
| Regex from https://stackoverflow.com/a/14693789 | ||
| """ | ||
|
|
||
| ansi_escape_re = re.compile(r"\x1B(?:[@-Z\\-_]|\[[0-?]*[ -/]*[@-~])") |
There was a problem hiding this comment.
all regex needs a comment on what we are trying to match. Even if we link to stackoverflow. Not a blocker
There was a problem hiding this comment.
I will add a task to the backlog to properly document regex's with examples and a humanly readable template of what we are trying to match.
| """ | ||
|
|
||
| progress_bar_re = re.compile( | ||
| r"^(.+?)\|(.+?)\|[ \#0-9]+?\|[ ]{1,3}[0-9]{1,2}\%" |
There was a problem hiding this comment.
please comment on what the regex is doing. for a quick read it is hard to understand. - not a blocker.
There was a problem hiding this comment.
I will add a task to the backlog to properly document regex's with examples and a humanly readable template of what we are trying to match.
This is a major rework of how we handle outputting information to the console. It consist of:
--verbose/--quietto adjust the verbosity of the output to the console.--log-to-fileto also save the stdout/stderr messages to files in addition to showing them on the console.--no-colorto disable colored output.Once this PR is merged, we should close #32 as this PR supersedes #32.