Skip to content

refactor(#159): rename sendSpoolDetectedMessage → sendOpenPrintTagMessage#162

Merged
sjordan0228 merged 1 commit into
devfrom
feature/rename-send-openprinttag
Apr 18, 2026
Merged

refactor(#159): rename sendSpoolDetectedMessage → sendOpenPrintTagMessage#162
sjordan0228 merged 1 commit into
devfrom
feature/rename-send-openprinttag

Conversation

@sjordan0228
Copy link
Copy Markdown
Contributor

@sjordan0228 sjordan0228 commented Apr 16, 2026

Closes #159.

Summary

  • Renames NFCManager::sendSpoolDetectedMessagesendOpenPrintTagMessage to reflect its actual scope (OpenPrintTag-only; uses opt_get_* CBOR accessors and sets tag_format = "OpenPrintTag").
  • 1 declaration + 1 definition + 9 call sites + 1 comment reference, all in src/NFCManager.{h,cpp}.
  • No behavior change. Pure rename.

Why

The old name implied it handled any spool-detection event. As we add Bambu / TigerTag / OpenSpool / OpenTag3D sender paths, the ambiguity makes the code harder to reason about. This rename makes it obvious which format the function belongs to and sets up per-format senders cleanly.

Test plan

  • Builds clean on esp32s3zero, esp32dev, esp32s3devkitc
  • review-remote (qwen3-coder:30b) — no issues
  • Physical test on ESP32-S3-DevKitC: OpenPrintTag scan works (display, LED, MQTT), other tag formats still route correctly

Summary by CodeRabbit

  • Refactor
    • Internal code improvements for enhanced maintainability. No user-facing functionality changes.

…sage

The function is OpenPrintTag-specific (uses opt_get_* CBOR accessors and
sets tag_format = "OpenPrintTag"). The old name implied it handled any
spool-detection event, which made multi-format support harder to reason
about. Rename clarifies scope ahead of per-format sender functions.

Mechanical rename: 1 declaration + 1 definition + 9 call sites (all in
NFCManager.{h,cpp}). No behavior change. Builds clean on esp32s3zero
and esp32dev.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 16, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 152ad216-680d-4e13-a147-18713330ca0d

📥 Commits

Reviewing files that changed from the base of the PR and between cb06259 and 47bdcfc.

📒 Files selected for processing (2)
  • src/NFCManager.cpp
  • src/NFCManager.h

📝 Walkthrough

Walkthrough

A refactoring change that renames the private method sendSpoolDetectedMessage to sendOpenPrintTagMessage across the NFCManager class declaration and implementation. The function is specific to OpenPrintTag format handling but previously had a misleadingly generic name. All call sites are consistently updated.

Changes

Cohort / File(s) Summary
Function rename and call site updates
src/NFCManager.h, src/NFCManager.cpp
Renamed private method sendSpoolDetectedMessage(bool suppress_spoolman_sync = false) to sendOpenPrintTagMessage(bool suppress_spoolman_sync = false). Updated all invocations across tag read, format, write verification, and notification dispatch paths (readAndParseTag, formatNewSpool, writeRawTag, executeAtomicWrite, executeWrite, processWriteQueue). Updated associated inline comments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main refactoring change: renaming sendSpoolDetectedMessage to sendOpenPrintTagMessage, with issue reference #159.
Description check ✅ Passed The description covers the Summary and Why sections well, includes a Test plan with verification steps, but lacks the Changes and How to Test sections specified in the template.
Linked Issues check ✅ Passed The pull request successfully addresses the primary objective from #159: renaming sendSpoolDetectedMessage to sendOpenPrintTagMessage and updating all call sites. The secondary objective (extracting a shared SpoolDetectedPayload builder) is appropriately deferred.
Out of Scope Changes check ✅ Passed All changes are strictly scoped to the rename refactoring in src/NFCManager.{h,cpp}. No unrelated modifications, behavioral changes, or out-of-scope additions are present.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/rename-send-openprinttag

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

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

@github-actions github-actions Bot added the size/S Small change (10-50 lines) label Apr 16, 2026
@sjordan0228 sjordan0228 merged commit eee5cb9 into dev Apr 18, 2026
3 checks passed
@sjordan0228 sjordan0228 deleted the feature/rename-send-openprinttag branch April 18, 2026 22:23
roomonthethird pushed a commit to roomonthethird/spoolsense_scanner that referenced this pull request Apr 22, 2026
Bumps FIRMWARE_VERSION from 1.7.2 → 1.7.3.

CHANGELOG.md:
- Adds [1.7.3] entry documenting SpoolSense#163 WiFi keep-awake, SpoolSense#165 case files,
  SpoolSense#164/SpoolSense#13 TigerTag partial writes, SpoolSense#159/SpoolSense#162 openprinttag rename.
- Backfills missing [1.7.2] entry (Bambu MIFARE Classic reading, SpoolSense#24).
- Backfills missing [1.7.1] entry (writer enrichment + bug fixes,
  SpoolSense#130/SpoolSense#101/SpoolSense#128/SpoolSense#151 and platform pin).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small change (10-50 lines)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant