Skip to content
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

Merged
merged 25 commits into from Jun 5, 2020

Conversation

JatinDev543
Copy link
Contributor

@JatinDev543 JatinDev543 commented May 29, 2020

This PR Modify lowercase typename for add/update mutation to camelCase.
Fixes #5380
Fixes #GRAPHQL-448


This change is Reviewable

Docs Preview: Dgraph Preview

@CLAassistant
Copy link

CLAassistant commented May 29, 2020

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label May 29, 2020
@JatinDev543 JatinDev543 changed the title Modified lowercase typename for add/update mutation to camelCase graphql: Modified lowercase typename for add/update mutation to camelCase May 29, 2020
@JatinDev543 JatinDev543 changed the title graphql: Modified lowercase typename for add/update mutation to camelCase graphql: Add camelCase for add/update mutation. May 29, 2020
Copy link
Contributor

@MichaelJCompton MichaelJCompton left a 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

Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a 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.

Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jatindevdg, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

Copy link
Contributor

@pawanrawal pawanrawal left a 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:]

Copy link
Contributor

@MichaelJCompton MichaelJCompton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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

@MichaelJCompton
Copy link
Contributor

[breaking] graphql: ....

@JatinDev543 JatinDev543 changed the title graphql: Add camelCase for add/update mutation. [breaking] graphql: Add camelCase for add/update mutation. Jun 3, 2020
@abhimanyusinghgaur abhimanyusinghgaur changed the title [breaking] graphql: Add camelCase for add/update mutation. [Breaking] graphql: Add camelCase for add/update mutation. Jun 5, 2020
@JatinDev543 JatinDev543 merged commit c5e3f78 into master Jun 5, 2020
@JatinDev543 JatinDev543 deleted the Jatin/graphql-448 branch June 5, 2020 21:53
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
…#5547)

This PR Modify lowercase typename for add/update mutation to camelCase.
Fixes dgraph-io#5380
Fixes #GRAPHQL-448

Co-authored-by: Abhimanyu Singh Gaur <12651351+abhimanyusinghgaur@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

(GraphQL API) Add/UpdateTypePayload use lowercase instead of camelCase for Type field
5 participants