refactor(heatsoaking): make command output more user-friendly

[tg73]

• New Features
  • Added VERBOSE and QUIET parameters to control output verbosity of heatsoak settings
  • Enhanced heatsoak settings display to show dynamic current state information, distinguishing between adaptive heatsoak mode with quality metrics and maximum duration, fixed heatsoak with duration, or no heatsoak configuration

[coderabbitai]

📝 Walkthrough

Walkthrough

The HEATSOAK_SETTINGS macro in the heatsoaking configuration file is enhanced to support VERBOSE and QUIET parameter flags, and the show action is refactored to dynamically inspect the current heatsoak state and output conditionally formatted messages based on whether adaptive, fixed, or no heatsoak is active.

Changes

Cohort / File(s)	Summary
Heatsoak macro parameter and output logic configuration/macros/heatsoaking.cfg	Added VERBOSE and QUIET parameter handling parsed from params2. Extended show action to dynamically inspect current RatOS macro state and conditionally output formatted status messages (adaptive with quality/duration, fixed duration, or no-soak) with optional verbose detail via SHOW_GCODE_VARIABLES. Adjusted parameter setting branch control flow.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• feat(macros): new commands to make temporary on-the-fly changes to heatsoak configuration easier #126: Directly related as it originally introduced the HEATSOAK_SETTINGS macro that this PR extends with new parameter handling and dynamic state inspection logic.

Poem

🐰 With VERBOSE flags we hop and shout,
Or QUIET down—no doubt, no doubt!
The heatsoak state we now inspect,
Each thermal dance we detect,
Parameters parsed with care and cheer! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: refactoring the heatsoaking macro to improve command output by adding dynamic state-based messages and VERBOSE/QUIET parameter handling for better user experience.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

configuration/macros/heatsoaking.cfg (1)

118-119: ⚠️ Potential issue | 🔴 Critical

Critical: Parameter handling broken for VERBOSE and QUIET.

Lines 118-119 use params|length to determine the action, but VERBOSE and QUIET have been popped from params2 (lines 100-101), not params. This causes incorrect branching:

• HEATSOAK_SETTINGS VERBOSE=1 → params|length is 1, so action becomes undefined (since params2.ACTION doesn't exist), enters the parameter-setting branch, but params2 is empty, so nothing happens. User gets no output.
• HEATSOAK_SETTINGS ACTION=show VERBOSE=1 → params|length is 2 (> 1), so line 119 raises "ACTION cannot be specified with other parameters" error, even though VERBOSE should be allowed with ACTION.

🔧 Proposed fix

-	{% set action = "show" if params|length == 0 else params2.ACTION %}
-	{% if action is defined and params|length > 1 %}
+	{% set action = params2.pop("ACTION", None) %}
+	{% if action is none %}
+		{% set action = "show" if params2|length == 0 else none %}
+	{% endif %}
+	{% if action is not none and params2|length > 0 %}
 		{ action_raise_error("ACTION cannot be specified with other parameters.") }
-	{% elif action is not defined %}
+	{% elif action is none %}
 		{% for k, v in params2.items() %}

This approach:

1. Pops ACTION from params2 alongside QUIET and VERBOSE
2. Defaults to "show" only if params2 is empty after popping control parameters
3. Uses params2|length for all branching decisions
4. Uses none checks instead of is defined for consistency with Jinja2 pop semantics

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@configuration/macros/heatsoaking.cfg` around lines 118 - 119, The
parameter-handling logic uses params|length and is defined checks but
ACTION/VERBOSE/QUIET were popped into params2; change the logic to pop ACTION
from params2 along with QUIET and VERBOSE, then compute action = "show" only if
params2 is empty after those pops, use params2|length for all branching
decisions (instead of params|length), and replace `is defined` checks with `is
not none`/`is none` checks consistent with Jinja2 pop semantics so that ACTION
may be combined with control flags like VERBOSE and QUIET.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@configuration/macros/heatsoaking.cfg`:
- Around line 118-119: The parameter-handling logic uses params|length and is
defined checks but ACTION/VERBOSE/QUIET were popped into params2; change the
logic to pop ACTION from params2 along with QUIET and VERBOSE, then compute
action = "show" only if params2 is empty after those pops, use params2|length
for all branching decisions (instead of params|length), and replace `is defined`
checks with `is not none`/`is none` checks consistent with Jinja2 pop semantics
so that ACTION may be combined with control flags like VERBOSE and QUIET.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a48eb86 and a99aae2.

📒 Files selected for processing (1)

• configuration/macros/heatsoaking.cfg

[tg73]

@coderabbitai
Re: #127 (review)
params2 is an alias for params, they reference the same object.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch the pull request review comments to find the specific review comment
gh pr view 127 --json reviews --jq '.reviews[] | select(.id == "3883703404") | {id: .id, body: .body, state: .state}'

# Also fetch review comments on the PR to see individual comments
gh api repos/Rat-OS/RatOS-configurator/pulls/127/comments --jq '.[] | select(.pull_request_review_id == 3883703404) | {id: .id, path: .path, line: .line, body: .body}'

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Try fetching all reviews to see their IDs
gh api repos/Rat-OS/RatOS-configurator/pulls/127/reviews --jq '.[] | {id: .id, user: .user.login, state: .state, body: .body}'

echo "---"

# Fetch all review comments
gh api repos/Rat-OS/RatOS-configurator/pulls/127/comments --jq '.[] | {id: .id, review_id: .pull_request_review_id, user: .user.login, path: .path, body: .body}'

Length of output: 5057

---

🏁 Script executed:

#!/bin/bash
# Read the heatsoaking.cfg file to see how params2 is defined
cat configuration/macros/heatsoaking.cfg | head -n 120

Length of output: 5750

---

@tg73, thank you for the clarification! You're absolutely right. I can see on line 99 that {% set params2 = params %} creates an alias (reference) to the same dictionary object, not a copy. This means when params2.pop("QUIET", None) and params2.pop("VERBOSE", None) are called on lines 100-101, they do indeed modify the underlying dictionary that params also references.

Therefore, the subsequent params|length checks on line 118 correctly reflect the state after QUIET and VERBOSE have been removed, and the code works as intended. My review comment was incorrect—there is no bug here. I apologize for the confusion.

The current implementation is correct, and no changes are needed for this aspect of the code.