Drop unused arguments instead of underscoring them#344
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR simplifies the ChangesBatch Operations API Simplification
User Command Output Simplification
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## parent/342-beta1-followups #344 +/- ##
==============================================================
- Coverage 71.34% 71.27% -0.07%
==============================================================
Files 553 552 -1
Lines 15350 15330 -20
==============================================================
- Hits 10951 10927 -24
- Misses 4399 4403 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Examples/MistDemo/Sources/MistDemo/Commands/CurrentUserCommand.swift (1)
47-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
--fieldshelp text appears stale after removing field filtering.
execute()now always outputs fulluserInfo, but help still says--fieldscontrols included fields. Please either remove/deprecate that option text here or wireconfig.fieldsinto the new output path so CLI behavior matches docs.Suggested doc fix (if option is intentionally removed)
- --fields <fields> Comma-separated list of fields to include --output-format <format> Output format: json, table, csv, yamlAlso applies to: 63-64
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Examples/MistDemo/Sources/MistDemo/Commands/CurrentUserCommand.swift` around lines 47 - 48, Help text for the --fields flag is stale because execute() always prints full userInfo; either remove/deprecate the flag from the CLI metadata or actually use config.fields when formatting output. Fix option by removing the "--fields" entry from the option/help definition (and any associated parsing) or implement wiring inside execute(): read config.fields (or the parsed fields option) and filter the userInfo before passing it to the output formatter (ensure the filtering logic uses the same field names as the help text). Update or remove any mentions of "--fields" in the command's help strings so documentation matches the runtime behavior.
🧹 Nitpick comments (1)
Sources/MistKit/Protocols/RecordManaging.swift (1)
47-49: ⚡ Quick winClarify batching responsibility in the protocol docs.
This comment currently implies
executeBatchOperationsitself enforces chunking. In this PR, concrete implementations (e.g.,CloudKitService) delegate directly, and batching is done by extension call sites. Consider rewording to avoid implying automatic batching at this layer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/MistKit/Protocols/RecordManaging.swift` around lines 47 - 49, The doc for executeBatchOperations misleadingly suggests the method enforces CloudKit's 200-ops chunking; update the documentation for executeBatchOperations (and mention RecordOperation) to clarify that it accepts a batch of RecordOperation but does not guarantee/request-level chunking—concrete implementations like CloudKitService or their callers are responsible for splitting into CloudKit-sized chunks; rephrase to state that batching/chunking is the caller/implementation responsibility and that executeBatchOperations processes the provided batch as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Examples/MistDemo/Sources/MistDemo/Commands/CurrentUserCommand.swift`:
- Around line 47-48: Help text for the --fields flag is stale because execute()
always prints full userInfo; either remove/deprecate the flag from the CLI
metadata or actually use config.fields when formatting output. Fix option by
removing the "--fields" entry from the option/help definition (and any
associated parsing) or implement wiring inside execute(): read config.fields (or
the parsed fields option) and filter the userInfo before passing it to the
output formatter (ensure the filtering logic uses the same field names as the
help text). Update or remove any mentions of "--fields" in the command's help
strings so documentation matches the runtime behavior.
---
Nitpick comments:
In `@Sources/MistKit/Protocols/RecordManaging.swift`:
- Around line 47-49: The doc for executeBatchOperations misleadingly suggests
the method enforces CloudKit's 200-ops chunking; update the documentation for
executeBatchOperations (and mention RecordOperation) to clarify that it accepts
a batch of RecordOperation but does not guarantee/request-level
chunking—concrete implementations like CloudKitService or their callers are
responsible for splitting into CloudKit-sized chunks; rephrase to state that
batching/chunking is the caller/implementation responsibility and that
executeBatchOperations processes the provided batch as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5afd57c1-9d76-45e9-93e8-bdff3755bd26
📒 Files selected for processing (13)
Examples/BushelCloud/Sources/BushelCloudKit/CloudKit/BushelCloudKitService.swiftExamples/BushelCloud/Tests/BushelCloudKitTests/CloudKit/MockCloudKitServiceTests.swiftExamples/BushelCloud/Tests/BushelCloudKitTests/ErrorHandling/CloudKitErrorHandlingTests.swiftExamples/BushelCloud/Tests/BushelCloudKitTests/Mocks/MockCloudKitService.swiftExamples/MistDemo/Sources/MistDemo/Commands/CurrentUserCommand.swiftSources/MistKit/Extensions/RecordManaging+Generic.swiftSources/MistKit/Extensions/RecordManaging+RecordCollection.swiftSources/MistKit/Protocols/RecordManaging.swiftSources/MistKit/Service/CloudKitService+RecordManaging.swiftTests/MistKitTests/Mocks/ResponseConfig.swiftTests/MistKitTests/Mocks/ResponseProvider.swiftTests/MistKitTests/Protocols/MockRecordManagingService.swiftTests/MistKitTests/Service/CloudKitServiceQueryTests+Helpers.swift
PR 343 silenced Periphery warnings by renaming unused parameters to `_:`. Where the design allows, drop the arguments entirely rather than keep them as dead placeholders. - `RecordManaging.executeBatchOperations(_:recordType:)` -> `executeBatchOperations(_:)`. Each `RecordOperation` already carries its own `recordType`, so the separate parameter was redundant. Updates the protocol, `CloudKitService` conformance, generic + record-collection extensions, and the in-tree mock. BushelCloud conformers (`BushelCloudKitService`, `MockCloudKitService`) are updated in lockstep; the s2s overload with `classification:` keeps `recordType:` because it actually logs the type. - `discoverAllUserIdentities`: delete the entire stubbed surface (`CloudKitService.discoverAllUserIdentities()`, the response-processor stub, and the `Operations.discoverAllUserIdentities.Input.Path` extension). The function was `@available(*, unavailable)` pending #28, which is unlikely to resolve. The generated OpenAPI client retains the endpoint and can be re-wrapped if the upstream behavior changes. - `setupServerToServer(apiToken:keyID:...)` -> drop `apiToken:`. Server-to-server uses key signing, not an API token. The dispatcher in `AuthenticationHelper.setupAuthentication` is specialized so only the s2s branch omits the argument. - `ResponseConfig.successfulQuery(records:)` / `ResponseProvider.successfulQuery(records:)` -> drop `records:`. The canned body was always `{"records": []}`. The query-tests helper that forwarded it (`makeSuccessfulService(records:)`) follows. - `CurrentUserCommand.filterUserFields` -> delete. The helper returned its input unchanged; `UserInfo` is fetched and handed directly to `outputResult`.
leogdion
force-pushed
the
claude/investigate-unused-arguments-1V9JY
branch
from
b3c9c25 to
338e6bb
Compare
Code ReviewOverview: This PR removes a genuinely redundant What is done well
Issues and Suggestions1. Empty-array fallback in let recordType = operations.first?.recordType ?? ""An empty guard !operations.isEmpty else { return }
let recordType = operations.first!.recordTypeOr at minimum use 2. Missing newline at end of The diff ends with "No newline at end of file". Minor but easy to fix. 3. New comment in Per project conventions (CLAUDE.md: "Only add a comment when the WHY is non-obvious"), the new two-line comment largely explains the obvious. If it survives, the first sentence (why 4. Before this PR, SummaryOverall this is a solid cleanup PR. The two main actionable items are the empty-array guard in
|
Code Review — PR #344: Drop unused arguments instead of underscoring themOverviewClean-up PR that removes redundant parameters flagged by Periphery. Three distinct changes:
The direction is correct throughout. Two items worth addressing before merge. Issues1. Empty-operations fallback in The new protocol-conforming implementation reads: let recordType = operations.first?.recordType ?? ""The empty-string fallback is harmless in practice (callers never pass an empty batch), but it silently passes guard !operations.isEmpty else { return }
let recordType = operations[0].recordTypeThis makes an accidental empty-batch a loud no-op rather than a silent log anomaly. 2. CLAUDE.md is now stale
Both should be removed. Minor Observations
SummaryTwo actionable items before merge:
Everything else looks good — the API simplification is correct, dead code is cleanly removed, and call-site updates are consistent across all conformers and tests. |
Empty batches now early-return instead of forwarding an empty recordType to the classification overload that logs it. Addresses PR #344 review.
|
Code Review for PR 344 - see inline notes below. |
|
Code Review for PR 344: Drop unused arguments instead of underscoring them Overview: Code Quality:
Potential Issues:
Breaking Change: Test Coverage: Summary: Solid cleanup, net -140 lines. Actionable items: (1) document the empty-array early-return guard; (2) verify config.fields reaches outputResult in CurrentUserCommand; (3) note the protocol breaking change in release notes. |
2 participants
Periphery flagged several arguments as unused. Rather than silencing
them with
_:, remove the arguments where the design allows.recordType:. EachRecordOperationalready carries its ownrecordType, so theseparate parameter was redundant. Updates protocol, CloudKitService
conformance, generic + record-collection extensions, and the
in-tree mock. BushelCloud conformers (
BushelCloudKitService,MockCloudKitService) are updated in lockstep; the s2s overloadwith
classification:keepsrecordType:because it logs the type.drop the unused
records:parameter; the canned body was always{"records": []}. The query-tests helper that forwarded it follows.returned its input unchanged. Also delete the dead duplicate
shouldIncludeFieldin the same file (the live copy lives inOutputFormatting+Records).
Summary by CodeRabbit