Skip to content

Reduce cyclomatic complexity in FieldValue+Codable, drop lint disable (#154)#414

Merged
leogdion merged 2 commits into
v1.0.0-beta.3from
refactor/cyclomatic-complexity-encoding
Jul 1, 2026
Merged

Reduce cyclomatic complexity in FieldValue+Codable, drop lint disable (#154)#414
leogdion merged 2 commits into
v1.0.0-beta.3from
refactor/cyclomatic-complexity-encoding

Conversation

@leogdion

Copy link
Copy Markdown
Member

Re-scope note

Issue #154 cited CustomFieldValue.swift / CustomFieldValuePayload.swiftthose files no longer exist on v1.0.0-beta.3. The remaining actionable hand-written // swiftlint:disable:next cyclomatic_complexity was in FieldValue+Codable.swift (encodeValue(to:)). The other disables live in generated Operations.*.Output.swift and are out of scope.

What

Split encodeValue(to:) into a thin dispatcher + encodeScalar/encodeComplex helpers so each stays under the complexity threshold and the disable is removed. Behavior, public API, and serialization output unchanged (date→ms math preserved).

Closes #154.

Verification

swift build, full swift test (538 pass), swiftlint --strict clean with the disable removed.

@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3bc550da-9ce0-438b-b9a8-a07362de7942

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/cyclomatic-complexity-encoding

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.

@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.26%. Comparing base (7791b09) to head (d9e65bb).

Files with missing lines Patch % Lines
...istKit/Models/FieldValues/FieldValue+Codable.swift 50.00% 10 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##           v1.0.0-beta.3     #414      +/-   ##
=================================================
+ Coverage          73.85%   74.26%   +0.40%     
=================================================
  Files                156      156              
  Lines               3698     3718      +20     
=================================================
+ Hits                2731     2761      +30     
+ Misses               967      957      -10     
Flag Coverage Δ
mistdemo-spm-macos 11.40% <50.00%> (+0.20%) ⬆️
mistdemo-swift-6.2-jammy 11.40% <50.00%> (+0.20%) ⬆️
mistdemo-swift-6.2-noble 11.40% <50.00%> (+0.20%) ⬆️
mistdemo-swift-6.3-jammy 11.51% <50.00%> (+0.31%) ⬆️
mistdemo-swift-6.3-noble 11.40% <50.00%> (+0.20%) ⬆️
spm 72.24% <50.00%> (+0.66%) ⬆️
swift-6.1-jammy 72.10% <50.00%> (+0.50%) ⬆️
swift-6.1-noble 72.42% <50.00%> (+0.77%) ⬆️
swift-6.2-jammy 72.12% <50.00%> (+0.52%) ⬆️
swift-6.2-noble 72.12% <50.00%> (+0.28%) ⬆️
swift-6.3-jammy 72.07% <50.00%> (+0.50%) ⬆️
swift-6.3-noble 72.04% <50.00%> (+0.50%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@claude

claude Bot commented Jun 30, 2026

Copy link
Copy Markdown

Code Review

Summary: Splits encodeValue(to:) into encodeScalar + encodeComplex to bring cyclomatic complexity under the SwiftLint threshold, removing the // swiftlint:disable:next comment.


Strengths

  • Behaviour is identical — the date→ms math, byte encoding, and all other cases are preserved exactly.
  • The defensive default: throw EncodingError.invalidValue(...) in encodeComplex is exactly the right safety net: if a new FieldValue case is added in the future and the author forgets to route it, the failure is loud and immediate rather than a silent no-op.
  • encodeValue now reads as a clean two-step dispatcher, which is easier to scan at a glance.

Minor observations

Bool return from encodeScalar

The Bool signal ("did I handle it?") works, but the pattern is slightly implicit for a new reader. A name like encodeIfScalar (returning Bool) or restructuring so encodeScalar handles only its own cases and encodeComplex is always called in the else-branch would be equally clear alternatives. Not a change request — just noting the tradeoff.

Comment on encodeScalar explains WHAT, not WHY

/// Encode the scalar cases (string, bytes, int64, double, date).
///
/// - Returns: `true` when `self` was a scalar case…

Per the project's CLAUDE.md, comments should explain the non-obvious WHY. The doc comment's Returns: note is useful because the Bool return is surprising at the call site, so this is borderline acceptable. But the first line is redundant with the function name and the switch body.

default: return false in encodeScalar

This is correct, but since FieldValue is likely not @frozen, the compiler won't warn if a new case is added without handling it here. The fail-loud throw in encodeComplex catches this at runtime; adding an #if DEBUG assertion or a comment explaining why encodeComplex is the backstop would make the intent explicit.


Overall: Minimal, correct, and closes the lint issue without any behavior change. The defensive default path in encodeComplex is a nice touch. LGTM.

…disable (#154)

Re-scoped from the now-removed CustomFieldValue(Payload).swift to the
remaining hand-written disable in FieldValue+Codable.swift. Extracted
helpers so the cyclomatic_complexity swiftlint disable can be removed;
behavior and serialization output unchanged. Generated Operations.*.Output
disables are out of scope.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@leogdion leogdion force-pushed the refactor/cyclomatic-complexity-encoding branch from da921b1 to f28213b Compare July 1, 2026 00:48
The #154 refactor split encodeValue into encodeScalar/encodeComplex but
added no tests; the only encode-path test covered .string. Add a
parameterized encode→decode round-trip over the scalar and complex cases
(driving the encodeScalar→encodeComplex delegation), plus explicit
encode-shape assertions for .date (milliseconds) and .bytes (string
payload), which do not round-trip by design.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@leogdion leogdion marked this pull request as ready for review July 1, 2026 15:03
@leogdion leogdion merged commit e2619d0 into v1.0.0-beta.3 Jul 1, 2026
70 of 71 checks passed
@leogdion leogdion deleted the refactor/cyclomatic-complexity-encoding branch July 1, 2026 15:16
@claude claude Bot mentioned this pull request Jul 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant