feat: Support uncommon profile structures #124
Conversation
|
Any reason why this is still a draft? It looks like these helper functions would cut down on boilerplate. |
ggreer
force-pushed
the
feat/support-uncommon-profile-structures
branch
from
cbb150d to
8e6c181
Compare
WalkthroughThe pull request introduces enhancements to the Changes
Sequence DiagramsequenceDiagram
participant Caller
participant ToProtoValue
participant StructPB
Caller->>ToProtoValue: Convert value
alt Supported Type
ToProtoValue->>StructPB: Create Value
StructPB-->>ToProtoValue: Return *structpb.Value
ToProtoValue-->>Caller: Return converted value
else Unsupported Type
ToProtoValue-->>Caller: Return error
end
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pkg/types/resource/role_trait.go (1)
15-23: Include more context in the error messageConsider including the failing key in the error message for better debugging. For example:
- return fmt.Errorf("error converting profile data: %w", err) + return fmt.Errorf("error converting profile data for key %q: %w", key, err)pkg/types/resource/traits.go (3)
45-62: Consider unifying nil checks across all GetProfile functions*Repeatedly checking for a nil profile and missing keys in each getter is prone to duplication. You could implement a small helper function to retrieve and type-assert fields, reducing this boilerplate.
64-81: Use a descriptive comment on the expected structure of listsWhile the logic is sound, clarifying the expected structure of the list elements (e.g., whether they can be nested structs or primitive values) would help future maintainers.
83-100: Document nested struct usage scenariosConsider documenting scenarios where nested structs are expected or how deeply nested data might be handled. This helps ensure consistent usage of GetProfileStructValue across the codebase.
pkg/types/resource/resource.go (1)
341-357: Expand numeric type coverage in ToProtoValueCurrently, the function only handles
intandfloat64. If the system needs to support other numeric types likeint64,uint64, orfloat32, adding explicit cases (or documentation) would prevent unexpected errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/types/resource/resource.go(2 hunks)pkg/types/resource/role_trait.go(1 hunks)pkg/types/resource/traits.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: go-test (1.22.x, windows-latest)
Summary by CodeRabbit
New Features
ToProtoValuefunction to convert various input types to Protocol Buffer valuesImprovements
WithRoleProfilefunction to provide more robust value conversion