refactor(custom-currency): refactor custom currency API type#4631
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR updates currency contracts across TypeSpec, generated JS client code, and Go runtime mappings to make ChangesCurrency schema and model rework
Dev shell tooling
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR refactors the custom currency API shape and generated clients. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "fix(custom-currency): fix review comment..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 (1)
api/spec/packages/aip/src/currencies/currency.tsp (1)
42-47: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTiny doc cleanup:
idis no longer shared here.The
CurrencyBasecomment still saysidis shared by both currency types, but this PR movesidontoCurrencyCustomonly.Proposed fix
/** * Describes the common properties of a currency. It is used as a base model for both standard currencies and custom currencies. - * It includes properties such as id, name, and symbol that are shared by both types of currencies. + * It includes properties such as type, name, and symbol that are shared by both types of currencies. */As per coding guidelines, “When refactoring or reverting code, preserve existing explanatory comments unless the code change makes them false, stale, or misleading.” As per path instructions, “The declared API should be accurate, in parity with the actual implementation, and easy to understand for the user.”
🤖 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 `@api/spec/packages/aip/src/currencies/currency.tsp` around lines 42 - 47, The CurrencyBase model comment is stale because it still claims id is shared by both currency types even though id now belongs only to CurrencyCustom. Update the docstring on CurrencyBase to remove id from the shared-property description while keeping the rest of the explanation aligned with the actual model shape. Use the CurrencyBase and CurrencyCustom symbols to verify the comments match the current API.Sources: Coding guidelines, Path instructions
🤖 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.
Inline comments:
In `@openmeter/currencies/adapter/currencies.go`:
- Line 31: The currency mapping in currencies adapter should preserve optional
semantics for Symbol instead of always wrapping the DB value with lo.ToPtr.
Update the mapping logic in the conversion function that builds the domain
currency model so that an empty DB Symbol becomes nil, while non-empty values
still become a pointer; this keeps the API type’s omitempty behavior and
distinguishes “unset” from “empty string”.
---
Outside diff comments:
In `@api/spec/packages/aip/src/currencies/currency.tsp`:
- Around line 42-47: The CurrencyBase model comment is stale because it still
claims id is shared by both currency types even though id now belongs only to
CurrencyCustom. Update the docstring on CurrencyBase to remove id from the
shared-property description while keeping the rest of the explanation aligned
with the actual model shape. Use the CurrencyBase and CurrencyCustom symbols to
verify the comments match the current API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 48a9eaf9-4a1f-45f8-82ce-5f2cfdd3d1e9
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (11)
api/spec/packages/aip-client-javascript/src/index.tsapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip/src/currencies/currency.tspapi/v3/api.gen.goapi/v3/handlers/currencies/convert.goflake.nixopenmeter/currencies/adapter/currencies.goopenmeter/currencies/models.goopenmeter/currencies/service/service.goopenmeter/currencies/service/service_test.go
| Code: c.Code, | ||
| Name: c.Name, | ||
| Symbol: c.Symbol, | ||
| Symbol: lo.ToPtr(c.Symbol), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Empty DB symbol becomes non-nil pointer to "".
lo.ToPtr(c.Symbol) unconditionally wraps the DB value, so if Symbol is stored as an empty string (now that it's meant to be optional), the domain model will have a non-nil *string pointing to "" rather than nil. That defeats the omitempty tag on the API type and downstream consumers can't distinguish "no symbol" from "symbol is empty string".
🩹 Suggested fix
- Symbol: lo.ToPtr(c.Symbol),
+ Symbol: lo.EmptyableToPtr(c.Symbol),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Symbol: lo.ToPtr(c.Symbol), | |
| Symbol: lo.EmptyableToPtr(c.Symbol), |
🤖 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 `@openmeter/currencies/adapter/currencies.go` at line 31, The currency mapping
in currencies adapter should preserve optional semantics for Symbol instead of
always wrapping the DB value with lo.ToPtr. Update the mapping logic in the
conversion function that builds the domain currency model so that an empty DB
Symbol becomes nil, while non-empty values still become a pointer; this keeps
the API type’s omitempty behavior and distinguishes “unset” from “empty string”.
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
borbelyr-kong
force-pushed
the
refactor/custom-currency
branch
from
d01bb96 to
10ea629
Compare
2 participants
Summary by CodeRabbit
Bug Fixes
symbolhandling across currency conversion and listing so returned data matches the optional nature ofsymbol.API Model Updates
idanddescription.Chores
yq-goto the default development environment tools.