Refactor _setup_cli_logging code#4734
Conversation
90a3c07 to
8ea5f6e
Compare
nicoddemus
left a comment
There was a problem hiding this comment.
Looks good, we also need a changelog entry. Seems to me like a "bugfix" about disabling logging automatically in case capture is also disabled, something which did not work previously anyway?
Codecov Report
@@ Coverage Diff @@
## master #4734 +/- ##
==========================================
- Coverage 95.65% 95.58% -0.08%
==========================================
Files 113 113
Lines 24968 24971 +3
Branches 2479 2480 +1
==========================================
- Hits 23884 23868 -16
- Misses 763 780 +17
- Partials 321 323 +2
Continue to review full report at Codecov.
|
8ea5f6e to
8c89741
Compare
I just had a look again and checked what happens if the capturemanager plugin is disabled. The live logging still works (because the _LiveLoggingStreamHandler supports that self.capture_manager is None.) So this PR actually does not fix a bug. Therefore, I decided not to write a bugfix changelog entry. I've adapted the code s.t. the current behavior is not changed. |
I agree, thanks for the explanation. 👍 |
Change the indentation in _setup_cli_logging by moving the self._log_cli_enabled check outside of the _setup_cli_logging method.
8c89741 to
7b8fd0c
Compare
Change the indentation in _setup_cli_logging by moving the
self._log_cli_enabled check outside of the _setup_cli_logging method.
Thanks for submitting a PR, your contribution is really appreciated!
Here's a quick checklist that should be present in PRs (you can delete this text from the final description, this is
just a guideline):
changelogfolder, with a name like<ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.masterbranch for bug fixes, documentation updates and trivial changes.featuresbranch for new features and removals/deprecations.Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:
AUTHORSin alphabetical order;