-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(schema): add convenience object & enum lookup methods #21549
Conversation
WalkthroughWalkthroughThe changes involve enhancements to schema type handling, specifically focusing on the lookup methods for object and enum types. The Changes
Sequence Diagram(s)sequenceDiagram
participant OldSchema
participant NewSchema
participant CompareModuleSchemas
participant Field
participant ModuleSchema
OldSchema->>CompareModuleSchemas: Compare Object Types
CompareModuleSchemas->>NewSchema: LookupObjectType
NewSchema-->>CompareModuleSchemas: Object Type Found/Not Found
CompareModuleSchemas->>Field: Validate Enum Types
Field->>ModuleSchema: LookupEnumType
ModuleSchema-->>Field: Enum Type Found/Not Found
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
Documentation and Community
|
@aaronc your pull request is missing a changelog! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
schema/module_schema_test.go (1)
Line range hint
190-198
: Approve renamingLookupType
toLookupObjectType
.The function name change clarifies that it specifically looks up object types.
Consider adding a type assertion or error handling for the returned type.
Removing the type assertion simplifies the code but may introduce a risk of failing silently if the returned type is not as expected.
To ensure the returned type is an
ObjectType
, consider adding a type assertion or error handling:objectType, ok := moduleSchema.LookupObjectType("object1") if !ok { t.Fatalf("expected to find object type \"object1\"") } var _ ObjectType = objectType // Assert that objectType is of type ObjectType if objectType.Name != "object1" { t.Fatalf("expected object type name \"object1\", got %q", objectType.Name) }Alternatively, you can use a type switch:
objectType, ok := moduleSchema.LookupObjectType("object1") if !ok { t.Fatalf("expected to find object type \"object1\"") } switch typ := objectType.(type) { case ObjectType: if typ.Name != "object1" { t.Fatalf("expected object type name \"object1\", got %q", typ.Name) } default: t.Fatalf("expected objectType to be of type ObjectType, got %T", typ) }
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (6)
- schema/diff/diff.go (3 hunks)
- schema/field.go (2 hunks)
- schema/module_schema.go (1 hunks)
- schema/module_schema_test.go (1 hunks)
- schema/testing/field.go (2 hunks)
- schema/type.go (3 hunks)
Additional context used
Path-based instructions (6)
schema/field.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/type.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/diff/diff.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/module_schema.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/testing/field.go (1)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.schema/module_schema_test.go (2)
Pattern
**/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go
: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (16)
schema/field.go (2)
41-43
: LGTM!The code changes improve the specificity of the type lookup and provide a clearer error message for missing enum types. The changes conform to the Uber Golang style guide.
72-72
: LGTM!The code change improves the specificity of the type lookup for enum field validation. The change conforms to the Uber Golang style guide.
schema/type.go (6)
20-21
: LGTM!The change to provide named return values for the
LookupType
method enhances clarity and conforms to the Uber Golang style guide.
22-24
: LGTM!The addition of the
LookupEnumType
method to theTypeSet
interface improves the type management functionality and conforms to the Uber Golang style guide.
25-26
: LGTM!The addition of the
LookupObjectType
method to theTypeSet
interface improves the type management functionality and conforms to the Uber Golang style guide.
35-38
: LGTM!The addition of the
EnumTypes
method to theTypeSet
interface improves the type management functionality and conforms to the Uber Golang style guide.
39-42
: LGTM!The addition of the
ObjectTypes
method to theTypeSet
interface improves the type management functionality and conforms to the Uber Golang style guide.
61-63
: LGTM!The implementations of the new methods in the
emptyTypeSet
struct maintain the expected behavior and conform to the Uber Golang style guide.Also applies to: 65-67, 71-72, 73-74
schema/diff/diff.go (4)
45-46
: LGTM!The code changes are approved. The change simplifies the code by directly using
LookupObjectType
instead ofLookupType
with type assertion, which is consistent with the Uber Golang style guide.
58-59
: LGTM!The code changes are approved. The change simplifies the code by directly using
LookupObjectType
instead ofLookupType
with type assertion, which is consistent with the Uber Golang style guide.
66-67
: LGTM!The code changes are approved. The change simplifies the code by directly using
LookupEnumType
instead ofLookupType
with type assertion, which is consistent with the Uber Golang style guide.
79-80
: LGTM!The code changes are approved. The change simplifies the code by directly using
LookupEnumType
instead ofLookupType
with type assertion, which is consistent with the Uber Golang style guide.schema/module_schema.go (2)
81-91
: LGTM!The
LookupEnumType
function is a useful addition to theModuleSchema
struct. It provides a convenient way to retrieve enum types by name, improving the usability of the schema.The function is correctly implemented, following the existing pattern of the
LookupType
function. It handles the case where the type is not found or is not an enum type by returning an emptyEnumType
andfalse
.The code changes are approved.
94-104
: LGTM!The
LookupObjectType
function is a useful addition to theModuleSchema
struct. It provides a convenient way to retrieve object types by name, improving the usability of the schema.The function is correctly implemented, following the existing pattern of the
LookupType
function. It handles the case where the type is not found or is not an object type by returning an emptyObjectType
andfalse
.The code changes are approved.
schema/testing/field.go (2)
23-23
: LGTM!The code changes are approved. The change simplifies the collection of enum types and improves performance by eliminating the need for a filtering function. The change also aligns with the Uber Golang style guide.
113-114
: LGTM!The code changes are approved. The change refines the lookup for enum types by using
typeSet.LookupEnumType
to directly check for the existence of the enum type. The change improves the clarity and efficiency of the code related to enum type handling. The change also aligns with the Uber Golang style guide.
Description
I ran into a bunch of cases where
LookupType
orAllTypes
needed to be called onTypeSet
and then type casting needed to happen at the call site. These utility functions simplify all that.TypeSet
has a private method so this is non-breaking and other similar utility functions can be added in the future.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
in the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
LookupEnumType
andLookupObjectType
.TypeSet
interface with additional methods for iterating over enum and object types.Improvements
Bug Fixes