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

Add location fields to type definitions #454

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

ssko1
Copy link

@ssko1 ssko1 commented Apr 29, 2021

Addresses #444

Overview

This change adds a Loc: errors.Location field to all definition structs defined in the types package. These struct fields are populated when the schema is parsed; the Line and Column fields in errors.Location represents the first character of the identifier for that definition.

For example:

directive @foo ON FIELD_DEFINITION # line: 1 column: 12
type Foo { }                       # line: 2 column: 6
input FooInput { }                 # line: 3 column: 7
# ...etc

This change strictly adds the Loc: errors.Location field to definitions that don't already have it. Definitions that already have location fields such as InputValueDefinition were not touched.

Motiviation and Examples

Tools built against a schema will most certainly need line/column source mapping as an additional form of identification when the name of a definition isn't enough.

For a specific example: as explained in #444, location data is important in our usage of graphql-go because a majority of schema validation occurs after parsing the schema (i.e. the schema syntax is valid, but there are other issues that aren't caught by the parser). Without location data on definitions, we won't be able to surface the lines and columns of domain-specific schema issues in our error messages.

Below are a few examples that I hope can showcase the importance of location data in definitions

Loc on ObjectTypeDefinition

Given the syntax-valid schema below:

type uncapitalizedtypename { 
  a: String
}

The Loc field in ObjectTypeDefinition will allow us to produce the following error:

schema.graphql:1:6: uncapitalizedtypename: Type names must be TitleCase.

Loc on FieldDefinition

type Foo {
  accountVerified: Boolean
}

The Loc field in FieldDefinition will allow us to produce the following error:

schema.graphql:2:3: accountVerified: Boolean field names should be prefixed by "is", "can", ...

@pavelnikolov pavelnikolov merged commit b8f211c into graph-gophers:master Apr 30, 2021
@pavelnikolov
Copy link
Member

Thank you for your contribution. The only thing that is kind of annoying is that the location is in the errors package but we already had it there for some types and not for all of them.

@ssko1
Copy link
Author

ssko1 commented May 1, 2021

@pavelnikolov Thank you for the quick merge!

I'm open to moving errors.Location somewhere else as its function is now no longer specifically tied to producing GraphQL errors. I'm hoping that, with the recent move towards semantic versioning, breaking changes such as moving the errors.Location type is less impactful and easier to reason about.

Could you explain a little more on your thoughts on how semantic versioning would work in this project? Would the master branch be open to experimental and/or frequent breaking changes?

@ssko1 ssko1 deleted the loc-types branch May 3, 2021 23:21
@pavelnikolov
Copy link
Member

Yes, master branch is open to experiments. Breaking changes are never a nice thing - we aim to avid them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants