feat: console output to a file #1632
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1632 +/- ##
==========================================
- Coverage 78.66% 78.63% -0.04%
==========================================
Files 296 296
Lines 6141 6173 +32
Branches 1002 1003 +1
==========================================
+ Hits 4831 4854 +23
- Misses 1093 1101 +8
- Partials 217 218 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
There was a problem hiding this comment.
@rhythmrx9 You need to update the test suite to show that the routine works with both a console and file output. output_console is explicitly called in test_output_engine - I suspect we don't want that now and I wonder if you should rename the fucntion _output_console to show that it is an internal function and shouldn't be called explicitly outside of the module,
|
You're right, the function should be renamed. Will add tests as well, thanks. |
| @@ -61,7 +61,23 @@ def format_version_range(version_info: VersionInfo) -> str: | |||
| return "-" | |||
|
|
|||
|
|
|||
| def output_console( | |||
| def output_console_wrap(*args): | |||
There was a problem hiding this comment.
Also, should this have a type hint for args?
All positional arguments have a different type, so |
| @@ -61,7 +61,23 @@ def format_version_range(version_info: VersionInfo) -> str: | |||
| return "-" | |||
|
|
|||
|
|
|||
| def output_console( | |||
| def output_console_wrap(*args): | |||
There was a problem hiding this comment.
Kind of nitpicky but... I think we should leave the "public" function as output_console in order to match output_html and the others. Makes a better API that way, although it's sort of an informal one. You could rename the internal one to _output_console_nowrap or something to make it more clear.
There was a problem hiding this comment.
Also, should this have a type hint for args?
|
resolved merge conflicts and updated the branch. |
closes #1631