-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[Breaking] graphql: Add camelCase for add/update mutation. #5547
Conversation
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.
Thank. Looks like the right idea. I'm suggesting to implement it just with some reslicing of the string, rather than taking an external dependency.
Also this'll need to fix up lots of tests. I'll pair you with someone to help you work out how to do that.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @jatindevdg, @manishrjain, @MichaelJCompton, @pawanrawal, and @vvbalaji-dgraph)
graphql/schema/gqlschema.go, line 28 at r1 (raw file):
"github.com/vektah/gqlparser/v2/gqlerror" "github.com/vektah/gqlparser/v2/parser" "github.com/iancoleman/strcase" //Added strcase for camelCase conversion
Let's not take a dependency on another package ... I think we can just do this by to lowering only the first letter, right? spilte the string into the first letter and the rest, tolowwer the first bit, add them back together.
graphql/schema/gqlschema.go, line 975 at r1 (raw file):
func addAddPayloadType(schema *ast.Schema, defn *ast.Definition) { qry := &ast.FieldDefinition{ Name: strcase.ToLowerCamel(defn.Name), //Converting Name to lower camelCase
no need for the comment
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.
Looks good, just got some minor comments related to formatting.
As this PR fixes two issues, one in GitHub and its equivalent in JIRA, the PR description should mention them like:
Fixes #5380
Fixes #GRAPHQL-448
No need to add a hyperlink explicitly.
Ans use the same PR description while doing Squash and Merge
, so that the PR will automatically get linked to the issue in JIRA board.
Reviewed 10 of 12 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @jatindevdg, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)
graphql/schema/gqlschema.go, line 24 at r2 (raw file):
"strings" "github.com/dgraph-io/dgraph/x" //Added strcase for camelCase conversion
Stale comment, need to remove.
graphql/schema/gqlschema.go, line 1643 at r2 (raw file):
return ok } func camelCase(x string) string {
Add a new line before this function.
A new line between two functions improves readability.
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.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jatindevdg, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)
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.
:lgtm, the camecase function can be simplified a bit
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jatindevdg, @manishrjain, and @vvbalaji-dgraph)
graphql/schema/gqlschema.go, line 1649 at r3 (raw file):
} return ((strings.ToLower(string(x[0]))) + x[1:])
The function could be simplified to
if len(x) == 0 {
return ""
}
return strings.ToLower(x[:1]) + x[1:]
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.
Reviewable status: 9 of 10 files reviewed, 4 unresolved discussions (waiting on @abhimanyusinghgaur, @jatindevdg, @manishrjain, and @vvbalaji-dgraph)
graphql/schema/gqlschema.go, line 24 at r2 (raw file):
"strings" "github.com/dgraph-io/dgraph/x" //Added strcase for camelCase conversion
no need for this comment
[breaking] graphql: .... |
…inc#5547) This PR Modify lowercase typename for add/update mutation to camelCase. Fixes hypermodeinc#5380 Fixes #GRAPHQL-448 Co-authored-by: Abhimanyu Singh Gaur <[email protected]>
This PR Modify lowercase typename for add/update mutation to camelCase.
Fixes #5380
Fixes #GRAPHQL-448
This change is
Docs Preview: