-
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
Added lang support #8924
Added lang support #8924
Conversation
@@ -2275,7 +2285,8 @@ func getNonIDFields(schema *ast.Schema, defn *ast.Definition, providesTypeMap ma | |||
return append(fldList, pd) | |||
} | |||
|
|||
func getFieldsWithoutIDType(schema *ast.Schema, defn *ast.Definition, providesTypeMap map[string]bool) ast.FieldList { | |||
func getFieldsWithoutIDType(schema *ast.Schema, defn *ast.Definition, | |||
providesTypeMap map[string]bool, isAddingInput bool) ast.FieldList { |
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.
What does the parameter isAddingInput
mean?
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, I see tests only for hash and exact index. We should add test for other indexes as well.
// We don't include fields in update patch, which corresponds to multiple language tags in dgraph | ||
// Example, nameHi_En: String @dgraph(pred:"Person.name@hi:en") | ||
// We don't add above field in update patch because it corresponds to multiple languages | ||
if isMultiLangField(fld, true) { |
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.
did we add a test for this case?
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.
Already present tests: graphql/e2e/directives/schema.graphql
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.
A couple of more comments and I see some old unresolved comments too. Please take a look. Thanks
@@ -1905,7 +1910,7 @@ func addAggregationResultType(schema *ast.Schema, defn *ast.Definition, provides | |||
} | |||
|
|||
// Adds titleMax, titleMin fields for a field of name title. | |||
if isOrderable(fld, defn, providesTypeMap) { | |||
if isOrderable(fld, defn, providesTypeMap) || isMultiLangField(fld, false) { |
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.
isOrderable function already checks for isMultiLandField. Do we need to do it again here?
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.
Yes, fails some tests
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.
why though?
dfdd0d3
to
bf5afe5
Compare
dfaf387
to
49cd7d0
Compare
49cd7d0
to
4841777
Compare
Added lang support to GraphQL
Added lang support to GraphQL