fixed color=false disables coloured output for log messages, not rest of the output - Issue #911#1779
Open
18c83fd3-25ea-4ed9-8205-2abeff9b3883 wants to merge 1 commit intoJoinMarket-Org:masterfrom
Conversation
Author
|
? |
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that the color = false setting in joinmarket.cfg now fully disables colored output for both log messages and direct console prints via jmprint().
- Introduces a global flag to track color output
- Updates
jmprint()to respect the color flag - Modifies
set_logging_color()to update the new flag
Comments suppressed due to low confidence (2)
src/jmbase/support.py:202
- The new
colored_outputflag and its effect onjmprint()should be documented in a module or function docstring so users understand howset_logging_colorinfluences both logging and direct prints.
def set_logging_color(colored=False):
src/jmbase/support.py:187
- Add unit tests for
jmprintto verify behavior withcolored_outputtoggled on and off, and in both TTY and non-TTY environments to prevent regressions.
if sys.stdout.isatty() and colored_output[0]:
| core_alert = [''] | ||
| debug_silence = [False] | ||
| # Global variable to track whether colored output is enabled | ||
| colored_output = [True] |
There was a problem hiding this comment.
[nitpick] Consider using a simple boolean global (e.g., colored_output_enabled: bool) with a global declaration in set_logging_color instead of a single-element list, which can be confusing for future maintainers.
Suggested change
| colored_output = [True] | |
| colored_output_enabled = True |
roshii
suggested changes
Oct 1, 2025
Contributor
roshii
left a comment
There was a problem hiding this comment.
Some tweaks are needed imo.
| .qt_for_python/ | ||
| cmtdata/ | ||
| **/build/ | ||
| test_venv/ |
| core_alert = [''] | ||
| debug_silence = [False] | ||
| # Global variable to track whether colored output is enabled | ||
| colored_output = [True] |
Contributor
There was a problem hiding this comment.
Simple boolean is best suited imo
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I've fixed the issue where setting color = false in joinmarket.cfg only disabled colored output for log messages but not for other console output.
The fix involved three changes to src/jmbase/support.py:
Added a global variable to track the color setting:
Global variable to track whether colored output is enabled
colored_output = [True]
Modified the jmprint() function to respect this setting:
if sys.stdout.isatty() and colored_output[0]:
print(jm_colorizer.colorize_message(fmtd_msg))
else:
print(fmtd_msg)
def set_logging_color(colored=False):
# Update the global colored_output setting
colored_output[0] = colored
if colored and sys.stdout.isatty():
handler.colorizer = jm_colorizer
else:
handler.colorizer = MonochromaticColorizer()
Now when a user sets color = false in joinmarket.cfg, it will disable colored output for both log messages and direct console output via jmprint().