Make getter return value optional

[heavyrubberslave]

Summary by CodeRabbit

• Refactor
  • Getter return types now explicitly indicate when a value may be absent, requiring callers to handle optional/missing values and improving robustness.
  • Write operations now only apply when a value is present, preventing unintended writes when data is unavailable.

[coderabbitai]

📝 Walkthrough

Walkthrough

Getter functions were changed to return std::optional<T> across the protocol and example code; BaseAttribute and its Getter alias now use std::optional, getValue() returns std::optional, and writeValue() only writes when a value is present. Example getters were updated to match the new signatures.

Changes

Cohort / File(s)	Summary
Protocol Interface Updates src/SlvCtrlProtocol.h	Changed BaseAttribute::Getter from T (*)(void*) to std::optional<T> (*)(void*); getValue() now returns std::optional<T> and writeValue() writes only when a value exists. Removed StrAttribute::writeValue override. Added <optional> and <type_traits> includes.
Example Code Updates examples/basic/src/main.cpp	Updated six getter functions (getSpeed, getMaxSpeed, getState, getReady, getBaud, getMode) to return std::optional<T> (including std::optional<const char*>); getState now conditionally returns std::nullopt when not ready. Setter signatures unchanged.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Memory optimizations #2: Modifies attribute getter/write API in SlvCtrlProtocol.h, aligning with the optional-based getter and write behavior shown here.

Poem

🐰 I hop and wrap each little get,

values tucked until they’re met.
Presence shown or gently naught,
optional keeps worries caught.
Hooray — safer hops and clever thought!

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 8.70% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ 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 summarizes the main change: converting getter return types to std::optional across the codebase.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into main

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

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/optional-getter-return-value

---

No actionable comments were generated in the recent review. 🎉

---

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]

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 (2)

src/SlvCtrlProtocol.h (2)

180-182: ⚠️ Potential issue | 🔴 Critical

Bug: std::optional<bool> in boolean context checks has_value(), not the contained value.

this->getValue() now returns std::optional<bool>. The ternary this->getValue() ? "true" : "false" invokes std::optional::operator bool(), which tests whether a value is present, not what the value is. Consequences:

Getter returns	has_value()	Output	Correct?
optional(true)	true	"true"	✅ coincidence
optional(false)	true	"true"	❌ should be "false"
nullopt	false	"false"	❌ misleading

This silently inverts false values to "true".

🐛 Proposed fix

     void writeValue(ISlvCtrlOut& out) const override {
-      out.write(this->getValue() ? "true" : "false");
+      auto val = this->getValue();
+      if (val.has_value()) {
+        out.write(val.value() ? "true" : "false");
+      }
     }

---

333-347: ⚠️ Potential issue | 🟡 Minor

StrAttribute now relies on base writeValue — beware of nullptr inside the optional.

With the removed writeValue override, BaseAttribute<const char*>::writeValue will call out.write(val.value()). If a getter returns std::optional<const char*>(nullptr) (i.e., has a value, but that value is nullptr), this passes nullptr to ISlvCtrlOut::write(const char*), which may cause undefined behavior depending on the implementation.

🤖 Fix all issues with AI agents

In `@src/SlvCtrlProtocol.h`:
- Around line 110-115: The current writeValue implementation silently omits
output when getValue() is nullopt which causes ambiguous empty values; change
the contract so absence is propagated: update writeValue(ISlvCtrlOut& out) to
return a bool (or optional) indicating whether a value was written (use
getValue() and if has_value() call out.write(...) and return true, else return
false), then update the caller cmdGet to check that return value and emit an
explicit sentinel (e.g., "null") or a non-ok status when writeValue returned
false; reference writeValue, getValue, cmdGet and ISlvCtrlOut::write when making
these changes.