-
Notifications
You must be signed in to change notification settings - Fork 191
fixed color=false disables coloured output for log messages, not rest of the output - Issue #911 #1779
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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] |
Copilot
AI
Jul 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
| colored_output = [True] | |
| colored_output_enabled = True |
roshii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tweaks are needed imo.
| .qt_for_python/ | ||
| cmtdata/ | ||
| **/build/ | ||
| test_venv/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be removed imo
| core_alert = [''] | ||
| debug_silence = [False] | ||
| # Global variable to track whether colored output is enabled | ||
| colored_output = [True] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple boolean is best suited imo
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().