Improve logging coloring and replace logging.success() #489
Conversation
WalkthroughReplaces the monkey-patched Logger.success with a registered custom log level Changes
Sequence Diagram(s)(Skipped — changes are registration of a logging level and call-site replacements; no new runtime control-flow worth diagramming.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flixopt/config.py (1)
23-31: SUCCESS level export and mapping look good; update method arg docs for completenessThe introduction of
SUCCESS_LEVEL(25), its export via__all__, and the explicit"SUCCESS"string handling inenable_console/enable_fileare all consistent with the new Logging docs and usage examples.Minor nit: the
Args:sections for bothLogging.enable_consoleandLogging.enable_filestill say:
level: Log level (DEBUG, INFO, WARNING, ERROR, CRITICAL or logging constant)Given you now explicitly support the
'SUCCESS'string (and document the SUCCESS level below), it would be clearer to mention it here as well, e.g.:
level: Log level (DEBUG, INFO, SUCCESS, WARNING, ERROR, CRITICAL or numeric level)This avoids users thinking SUCCESS is only available numerically.
Also applies to: 220-257, 283-323, 391-395
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)flixopt/calculation.py(3 hunks)flixopt/config.py(7 hunks)flixopt/network_app.py(2 hunks)flixopt/results.py(2 hunks)tests/test_config.py(0 hunks)
💤 Files with no reviewable changes (1)
- tests/test_config.py
🧰 Additional context used
🧬 Code graph analysis (2)
flixopt/calculation.py (1)
flixopt/config.py (1)
CONFIG(162-719)
flixopt/results.py (1)
flixopt/config.py (1)
CONFIG(162-719)
🔇 Additional comments (4)
flixopt/config.py (1)
96-105: ColoredMultilineFormatter box-color changes preserve structureThe updated
ColoredMultilineFormatter.format()now applies color codes to the box characters (┌─,│,└─) while keeping the timestamp dimmed and the alignment identical (sameindentand padding).The string assembly still mirrors the non-colored formatter’s layout, so this should only affect aesthetics, not readability or parsing.
Also applies to: 108-125
flixopt/results.py (1)
18-19: Consistent use of SUCCESS_LEVEL for results-saving logImporting
SUCCESS_LEVELhere and switching the final message inCalculationResults.to_file()to:logger.log(SUCCESS_LEVEL, f'Saved calculation results "{name}" to {paths.model_documentation.parent}')keeps success logging consistent with the new logging API while preserving existing behavior and timing of the message.
Also applies to: 1098-1099
flixopt/network_app.py (1)
21-23: Network visualization start message correctly migrated to SUCCESS_LEVELUsing the shared
SUCCESS_LEVELconstant here:logger.log(SUCCESS_LEVEL, f'Network visualization started on http://127.0.0.1:{port}/')keeps the visualization “startup” message aligned with the project’s success-level convention and shares the same tuning knobs as other success logs.
Also applies to: 784-785
flixopt/calculation.py (1)
25-30: Calculation success logs unified on SUCCESS_LEVELImporting
SUCCESS_LEVELand switching both:
FullCalculation.solvecompletion log, andSegmentedCalculation.do_modeling_and_solveaggregated completion logto
logger.log(SUCCESS_LEVEL, ...)keeps the wording and timing of the messages but unifies them under the shared success log level defined inconfig.py. This matches how other modules now emit success messages.Also applies to: 245-247, 676-677
…to capture expected warnings, preventing them from appearing in the test output. What was fixed: 1. test_both_old_and_new_raises_error (lines 122-174): Wrapped each test case with pytest.warns() to capture the deprecation warnings that occur before the ValueError is raised. 2. test_optional_parameter_deprecation (lines 289-306): Changed from pytest.warns() to warnings.catch_warnings(record=True) because the optional parameter triggers two separate deprecation warnings (one from interface.py and one from structure.py). 3. test_both_optional_and_mandatory_no_error (lines 325-349): Same fix as above - using warnings.catch_warnings() to capture both warnings. The warnings were showing up because: - Some tests used pytest.raises() without capturing the warnings that occurred before the exception - The optional parameter uniquely triggers two warnings from different code locations, and pytest.warns() with a match pattern only catches one
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/test_invest_parameters_deprecation.py (2)
292-299: Optional parameter deprecation tests are flexible but comment and assertion are slightly out of sync.The use of
warnings.catch_warnings(record=True)plusany(issubclass(..., DeprecationWarning))is a good, non‑brittle way to allow multiple deprecation sources while only requiring at least one warning. However, the comment “Two warnings are expected (one from interface.py, one from structure.py)” suggests an exact count that the assertion no longer enforces.Consider either:
- Tightening the assertion to check for
>= 2deprecation warnings, or- Relaxing the comment wording (e.g. “Currently two warnings are emitted... at least one is required by this test”) to avoid confusion.
Also applies to: 301-307
334-341: Same note about warning count expectations for combined optional/mandatory.Here as well, the comment says “Two warnings are expected (one from interface.py, one from structure.py)” while the logic only asserts that at least one
DeprecationWarningis present. The pattern itself is fine and keeps the test future‑proof; the mismatch is only in expectations vs. assertions.Mirroring the earlier block, consider either enforcing
>= 2deprecation warnings or softening the comment so it doesn’t imply an exact count.Also applies to: 343-349
tests/test_config.py (2)
97-108: Be explicit about whether the numeric value 25 is part of the public contractBoth
test_success_level_numericand theassert SUCCESS_LEVEL == 25intest_success_level_constanthard-code the numeric value. This is fine if you intend “SUCCESS is exactly level 25” to be a stable public API guarantee; otherwise, these tests will make future renumbering noisy.If you’d rather keep the numeric free to change, consider:
- Dropping
assert SUCCESS_LEVEL == 25, and/or- Using
SUCCESS_LEVELinstead of literal25intest_success_level_numeric.
125-139: Color customization test only asserts message presence; consider (optionally) asserting color config
test_success_color_customizationcurrently just checks that the SUCCESS message appears, which is robust but doesn’t actually verify thatset_colorsupdated anything.If you want a stronger, still-reasonable check, you could optionally extend this test to introspect the formatter when
colorlogis available, e.g. (guarded by a flag/skip):
- After
set_colors(...), find the console handler with aColoredMultilineFormatter.- Assert that
formatter.log_colors['SUCCESS'] == 'bold_green,bg_black'.That would validate the color mapping without relying on terminal escape sequences, while keeping behavior when
colorlogis missing unchanged (via a conditional orpytest.skip).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_config.py(2 hunks)tests/test_invest_parameters_deprecation.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_config.py (1)
flixopt/config.py (6)
CONFIG(162-719)MultilineFormatter(36-77)Logging(189-483)enable_console(283-357)enable_file(360-411)set_colors(427-483)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
🔇 Additional comments (4)
tests/test_invest_parameters_deprecation.py (1)
125-132: Good coverage of “old + new” conflict paths (warning + ValueError).Using
pytest.warns(DeprecationWarning, ...)wrapped aroundpytest.raises(ValueError, ...)nicely asserts that a deprecation warning is emitted in addition to the conflict error for each parameter pair. This is a robust, idiomatic pattern in pytest and matches the intended behavior change.Also applies to: 135-143, 146-153, 166-174
tests/test_config.py (3)
8-8: SUCCESS_LEVEL import and basic SUCCESS logging usage look correctUsing the exported
SUCCESS_LEVELconstant andlogger.log(SUCCESS_LEVEL, ...)intest_custom_success_levelaligns with the new logging API and ensures the test is tied to the public symbol instead of any monkey-patched method. No issues here.Also applies to: 75-80
81-96: Good coverage of SUCCESS as an effective minimum log level
test_success_level_as_minimumcorrectly exercises the threshold behavior: INFO suppressed, SUCCESS logged, WARNING logged. This accurately reflects the logger level semantics with a custom level at 25 and will guard against regressions in the string-to-level mapping for"SUCCESS".
110-123: SUCCESS file logging behavior is well exercised
test_success_file_loggingnicely validates that file logging respects the SUCCESS threshold (INFO filtered, SUCCESS persisted) and that the RotatingFileHandler writes what you expect. The structure is clear and should be stable.
…() and enable_file()
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
flixopt/config.py (1)
722-738: Optional: mention SUCCESS in deprecated helper docstring
change_logging_level()ultimately callsCONFIG.Logging.enable_console(), which now supports the customSUCCESSlevel via the string'SUCCESS'. The docstring’sArgsdescription still only lists the standard levels; if you keep this helper around until removal, consider updating the text to mentionSUCCESSas well for consistency with the rest of the logging docs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)flixopt/config.py(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.11)
- GitHub Check: test (3.10)
- GitHub Check: test (3.13)
- GitHub Check: test (3.12)
🔇 Additional comments (7)
CHANGELOG.md (1)
58-66: SUCCESS level and coloring entry matches implementationThe Unreleased notes correctly describe
SUCCESS_LEVELand its usage vialogger.log(SUCCESS_LEVEL, ...), and the “logger coloring improved” line aligns with the formatter changes inflixopt/config.py. No changes needed here.flixopt/config.py (6)
9-24: Typing imports and public SUCCESS_LEVEL export are consistentUsing
TYPE_CHECKINGwithTextIOandfrom __future__ import annotationskeeps runtime imports light while preserving good type information. AddingSUCCESS_LEVELto__all__matches how it’s used elsewhere and cleanly exposes the new level as part of the public API.
80-125: Colored multiline formatter spacing change looks correctThe updated
ColoredMultilineFormatter.format()keeps the visual structure aligned withMultilineFormatterwhile adding per-level coloring. The indentation and box borders (┌─,│,└─) are consistent between single- and multi-line branches, and there’s no impact on logging semantics.
220-257: SUCCESS level documentation and examples are accurateThe new “Log Levels” subsection and examples correctly describe
SUCCESSas level 25 between INFO and WARNING and demonstrate the intended usage viaSUCCESS_LEVELandlogger.log(...). The color customization snippet also matches the behavior ofset_colors.
283-323: SUCCESS handling in enable_console is robustExtending
enable_consoleto accept'SUCCESS'(case/whitespace-insensitive) and map it toSUCCESS_LEVEL, while leaving numeric levels unchanged, cleanly integrates the custom level without altering existing behavior for standard levels.
360-401: enable_file SUCCESS support mirrors console behaviorThe same
'SUCCESS'→SUCCESS_LEVELmapping inenable_file, combined with the existing handler reset and formatter setup, ensures file logging supports the custom level in a predictable way and remains symmetric with console configuration.
545-550: Narrowing default_engine to Literal improves type safetyTyping
CONFIG.Plotting.default_engineasLiteral['plotly', 'matplotlib']matches the documented options and helps catch invalid assignments at type-check time without changing runtime behavior.
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
New Features
Improvements
Behavior Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.