Parser for type declaration.

[martinmr]

Add a parser for type declarations along with the schema.
Results are ignored for now.

---

This change is 

[srfrog]

Reviewable status: 0 of 10 files reviewed, 7 unresolved discussions (waiting on @martinmr and @srfrog)

---

protos/pb.proto, line 358 at r1 (raw file):

  repeated TypeField fields = 2;
}

IMO a type is a collection of schema fields, could we perhaps reuse SchemaUpdate as repeated with a name? e.g.,

message SchemaType {
   string name = 1;
   repeated SchemaUpdate fields = 2;
}

or even create a new object SchemaField and derive SchemaType and SchemaUpdate from that.

---

schema/parse.go, line 304 at r1 (raw file):

	it.Next()
	typeUpdate := &pb.TypeUpdate{Name: it.Item().Val}
	log.Printf("Name: %v\n", it.Item().Val)

extra debug log

---

schema/parse.go, line 372 at r1 (raw file):

		if it.Item().Typ == itemExclamationMark {
			field.NonNullableList = true

maybe NonNullableList and NonNullable can be combined? List and NonNullable seem to be disjointed.

---

schema/parse.go, line 417 at r1 (raw file):

		return false
	}

group these if's with a switch without default case. cheaper and faster.

---

schema/parse.go, line 430 at r1 (raw file):

	l.Run(lexText)
	if err := l.ValidateResult(); err != nil {
		return result, err

you should return nil with the error

---

schema/parse.go, line 448 at r1 (raw file):

				typeUpdate, err := parseTypeDeclaration(it)
				if err != nil {
					return result, err

return nil with error

---

schema/parse.go, line 456 at r1 (raw file):

			schema, err := parseScalarPair(it, item.Val)
			if err != nil {
				return result, err

nil with error again and the others below...

[martinmr]

Reviewable status: 0 of 10 files reviewed, 7 unresolved discussions (waiting on @srfrog)

---

protos/pb.proto, line 358 at r1 (raw file):

Previously, srfrog (Gus) wrote…

IMO a type is a collection of schema fields, could we perhaps reuse SchemaUpdate as repeated with a name? e.g.,

message SchemaType {
   string name = 1;
   repeated SchemaUpdate fields = 2;
}

or even create a new object SchemaField and derive SchemaType and SchemaUpdate from that.

Done. Move the fields into Schema Update.

---

schema/parse.go, line 304 at r1 (raw file):

Previously, srfrog (Gus) wrote…

extra debug log

Done.

---

schema/parse.go, line 372 at r1 (raw file):

Previously, srfrog (Gus) wrote…

maybe NonNullableList and NonNullable can be combined? List and NonNullable seem to be disjointed.

They cannot be combined. Graphql allows you to specify fields of type [String]! or [String!]! or [String!] so I need the two fields to support all combinations.

---

schema/parse.go, line 417 at r1 (raw file):

Previously, srfrog (Gus) wrote…

group these if's with a switch without default case. cheaper and faster.

can't really be done because the ifs are comparing different members of the array.

---

schema/parse.go, line 430 at r1 (raw file):

Previously, srfrog (Gus) wrote…

you should return nil with the error

Can't return nil as a struct but I changed the code to explicitly return an empty struct.

---

schema/parse.go, line 448 at r1 (raw file):

Previously, srfrog (Gus) wrote…

return nil with error

Done. See above.

---

schema/parse.go, line 456 at r1 (raw file):

Previously, srfrog (Gus) wrote…

nil with error again and the others below...

Done. See above.

Addressed comments

[srfrog]

Reviewed 3 of 10 files at r1, 2 of 4 files at r2.
Reviewable status: 5 of 10 files reviewed, 2 unresolved discussions (waiting on @martinmr)

---

schema/parse.go, line 417 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

can't really be done because the ifs are comparing different members of the array.

Sorry I should had been clearer, this is what I meant:

switch {
case err != nil || len(nextItems) != 2:
  return false
 
case nextItems[0].Typ != itemText:
  return false

case nextItems[1].Typ != itemLeftCurl:
  return false
}

---

schema/parse.go, line 430 at r1 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

Can't return nil as a struct but I changed the code to explicitly return an empty struct.

Actually it would be better if you return a pointer in Parse() then return nil. Then your branch tests use nil which is easier.
e.g., https://play.golang.org/p/2TYMEQ6Q3jK

[martinmr]

Reviewable status: 3 of 11 files reviewed, 2 unresolved discussions (waiting on @srfrog)

---

schema/parse.go, line 417 at r1 (raw file):

Previously, srfrog (Gus) wrote…

Sorry I should had been clearer, this is what I meant:

switch {
case err != nil || len(nextItems) != 2:
  return false
 
case nextItems[0].Typ != itemText:
  return false

case nextItems[1].Typ != itemLeftCurl:
  return false
}

Done.

---

schema/parse.go, line 430 at r1 (raw file):

Previously, srfrog (Gus) wrote…

Actually it would be better if you return a pointer in Parse() then return nil. Then your branch tests use nil which is easier.
e.g., https://play.golang.org/p/2TYMEQ6Q3jK

Done.

[srfrog]

Reviewed 3 of 5 files at r3.
Reviewable status: 6 of 11 files reviewed, all discussions resolved

[srfrog]

Reviewable status: 6 of 11 files reviewed, 3 unresolved discussions (waiting on @martinmr)

---

schema/parse.go, line 334 at r3 (raw file):

func parseTypeField(it *lex.ItemIterator) (*pb.SchemaUpdate, error) {
	field := &pb.SchemaUpdate{Predicate: it.Item().Val}
	list := false

use var list bool instead, zero value is false

---

schema/parse.go, line 351 at r3 (raw file):

	}
	typ := getType(it.Item().Val)
	field.ValueType = typ

can you combine these two and use it below? typ is only used once.

---

schema/parse.go, line 422 at r3 (raw file):

	var result SchemasAndTypes
	var schemas []*pb.SchemaUpdate
	var types []*pb.TypeUpdate

I dont think you need schemas and types, you have zero-value result so you can use the fields directly.
e.g.,

err := resolveTokenizers(result.Schemas)
// ...
result.Types = append(result.Types, typeUpdate)

[martinmr]

Reviewable status: 6 of 11 files reviewed, 3 unresolved discussions (waiting on @srfrog)

---

schema/parse.go, line 334 at r3 (raw file):

Previously, srfrog (Gus) wrote…

use var list bool instead, zero value is false

Done.

---

schema/parse.go, line 351 at r3 (raw file):

Previously, srfrog (Gus) wrote…

can you combine these two and use it below? typ is only used once.

Done.

---

schema/parse.go, line 422 at r3 (raw file):

Previously, srfrog (Gus) wrote…

I dont think you need schemas and types, you have zero-value result so you can use the fields directly.
e.g.,

err := resolveTokenizers(result.Schemas)
// ...
result.Types = append(result.Types, typeUpdate)

Done.

[srfrog]

Reviewable status: 6 of 11 files reviewed, all discussions resolved

[manishrjain]

Couple of comments. Address them and it's good to go.

Reviewed 6 of 10 files at r1, 4 of 5 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @martinmr)

---

protos/pb.proto, line 346 at r1 (raw file):

message TypeField {
  string name = 1;

string predicate = 1;

---

protos/pb.proto, line 355 at r1 (raw file):

message TypeUpdate {
  string name = 1;

string type_name?

---

protos/pb.proto, line 344 at r4 (raw file):

	bool non_nullable = 10;
	bool non_nullable_list = 11;
	string object_type_name = 12;

Add a comment about this.

---

schema/parse.go, line 305 at r4 (raw file):

	// Call next again to skip the { character.
	it.Next()

Shouldn't you check that the next char is in fact left curl? If someone puts a comma here, we would want to return an error.

---

schema/parse.go, line 314 at r4 (raw file):

			it.Next()
			if it.Item().Typ != itemNewLine {
				return nil, x.Errorf("Expected new line after type declaration. Got %v",

I thought Lucas had worked on a change introducing a lex error field, which would put the line number, etc. in the error log. Maybe use that.

[martinmr]

Reviewable status: 7 of 11 files reviewed, 5 unresolved discussions (waiting on @manishrjain and @srfrog)

---

protos/pb.proto, line 346 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

string predicate = 1;

Done. Moved these fields to SchemaUpdate

---

protos/pb.proto, line 355 at r1 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

string type_name?

Done.

---

protos/pb.proto, line 344 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Add a comment about this.

Done.

---

schema/parse.go, line 305 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

Shouldn't you check that the next char is in fact left curl? If someone puts a comma here, we would want to return an error.

I didn't add the check here because the check is performed by isTypeDeclaration but I have added them here as well just in case.

---

schema/parse.go, line 314 at r4 (raw file):

Previously, manishrjain (Manish R Jain) wrote…

I thought Lucas had worked on a change introducing a lex error field, which would put the line number, etc. in the error log. Maybe use that.

That is only being used inside the lexer and it's not being used at all inside this file. I'll look into it but that should probably be done in a different PR.