Naming #20

bsr203 · 2015-10-11T20:48:23Z

I am just starting with the library and my comments may be premature. I haven't touched Go for an year now, so not sure the community has changed their view on conventions. But I found the naming is bit verbose, and add noise.

If I look at the types, we know the context is GraphQL type, so not sure we need to attach GraphQL to everything

GraphQLObjectType -> ObjectType or Object
GraphQLConnectionDefinitions -> ConnectionDefinitions
GraphQLSchema -> Schema
GraphQLResolveInfo -> ResolveInfo
GraphQLString -> String
...

GraphQLFieldConfigMap -> FieldConfig

as we don't need to add the data type it holds to.

Also quickly looking through the doc

type GraphQLInterfaceType
func NewGraphQLInterfaceType(config GraphQLInterfaceTypeConfig) *GraphQLInterfaceType
func (it *GraphQLInterfaceType) AddFieldConfig(fieldName string, fieldConfig *GraphQLFieldConfig)
func (it *GraphQLInterfaceType) GetDescription() string
func (it *GraphQLInterfaceType) GetError() error
func (it *GraphQLInterfaceType) GetFields() (fields GraphQLFieldDefinitionMap)
func (it *GraphQLInterfaceType) GetName() string
func (it *GraphQLInterfaceType) GetObjectType(value interface{}, info GraphQLResolveInfo) *GraphQLObjectType
func (it *GraphQLInterfaceType) GetPossibleTypes() []*GraphQLObjectType

typically you don't prefix the getters with Get, so they will be like Description(), Error() , Name(), or may even export the fields.

Let me know yours and @sogko 's thoughts on this. I could try a PR for if you agree.

thanks
bsr.

The text was updated successfully, but these errors were encountered:

sogko · 2015-10-12T02:29:38Z

Hi @bsr203

Thanks a lot for taking your time to look into this 👍🏻

Struct names for GraphQL types

I do agree with you that currently the API is pretty verbose. @chris-ramon and I had a discussion previously about the package and type names in PR #16. Seems like all of us are on the same page about having a more concise API, which is great! I don't think anyone has started on working on it yet.

Getters method names

Regarding the getters, it gets a bit tricky, because those are interface methods, for e.g.
type GraphQLInterfaceType implements GraphQLType, GraphQLAbstractType, etc,

So for e.g, currently, GraphQLType and GraphQLAbstractType interfaces are defined as the following, enforcing structs that implement it to export the following methods:

type GraphQLType interface {
    GetName() string
    GetDescription() string
    GetError() error
    String() string
}
type GraphQLAbstractType interface {
    GetObjectType(value interface{}, info GraphQLResolveInfo) *GraphQLObjectType
    GetPossibleTypes() []*GraphQLObjectType
    IsPossibleType(ttype *GraphQLObjectType) bool
}

And types that implement it, for e.g. GraphQLInterfaceType exposed the following fields and methods:

type GraphQLInterfaceType struct {
    Name        string `json:"name"`
    Description string `json:"description"`
    ResolveType ResolveTypeFn
}
// implements `GraphQLType` interface
func (it *GraphQLInterfaceType) GetDescription() string {}
func (it *GraphQLInterfaceType) GetError() error {}
func (it *GraphQLInterfaceType) GetName() string {}
// implements `GraphQLAbstractType` interface
func (it *GraphQLInterfaceType) GetObjectType(value interface{}, info GraphQLResolveInfo)  {}*GraphQLObjectType
func (it *GraphQLInterfaceType) GetPossibleTypes() []*GraphQLObjectType {}

What we have right now

So in practice, what we have currently is the following:

If the user knows that a variable is of GraphQLInterfaceType, he can access exported fields easily, includingName, Description and other fields specific to only GraphQLInterfaceType , for e.g.:

var node types.GraphQLInterfaceType
...
name := node.Name // access field directly, you can even set it.
name2 := node.GetName() // or you can choose to access the field from its interface method.
resolveFn := node.ResolveFn // exported field specific to `GraphQLInterfaceType`

If the user only care that a variable is of GraphQLType interface, and wants its name, he gets it from the interface method

var node types.GraphQLType
...
name := node.GetName() // access the field from its interface method.

Considering an alternate `GraphQLType` interface

At one point of time, I did try to consider the following interface for GraphQLType:

type GraphQLType interface {
    Name() string
    Description() string
    Error() error
    String() string
}

But we'll face a couple of issues:

GraphQLInterfaceType won't be able to export Name and Description fields, it will collide with the interface methods. (Contrived example: http://play.golang.org/p/dD_qelhoOU)
That means we might need to make the Name, Description fields private.
If we make Name, Description fields private, we would have a weird convention to access GraphQLInterfaceType fields, for e.g:

type GraphQLInterfaceType struct {
    // private fields, with getter methods
    name        string `json:"name"`
    description string `json:"description"`
        // public field
    ResolveType ResolveTypeFn
}
// implements `GraphQLType` interface
func (it *GraphQLInterfaceType) Name() string {}
func (it *GraphQLInterfaceType) Description() string {}

...

var node types. GraphQLInterfaceType

// to access name, user has to access it through its method
name := node.Name()

// to access a different field, ResolveFn for e.g, user access it directly
resolveFn := node.ResolveFn

So what we will have are two ways to access field values for GraphQLInterfaceType;

direct access and,
through methods.

It'll make it frustrating experience for the user, in my humble opinion.

On another hand, this won't be so bad if we choose to restrict direct field access by design, e.g

type GraphQLInterfaceType struct {
    // private fields, with getter methods
    name        string `json:"name"`
    description string `json:"description"`
    resolveType ResolveTypeFn
}
// implements `GraphQLType` interface
func (it *GraphQLInterfaceType) Name() string {}
func (it *GraphQLInterfaceType) Description() string {}
func (it *GraphQLInterfaceType) ResolveType() ResolveTypeFn {}

...

var node types. GraphQLInterfaceType

// to access fields, user has to access it through its method
name := node.Name()
resolveFn := node.ResolveFn()
...

It is possible to design it that way, but I'm not sure if this is an overkill.
Maybe there is a better and idiomatic way to define the interface probably, would love input on this.

Appreciate any thoughts / feedback from anyone on this 👍🏻
Thanks guys!

bsr203 · 2015-10-12T04:01:54Z

@sogko thanks for the kind words and the detailed analysis. I couldn't agree with you guys more on the discussion https://github.com/chris-ramon/graphql-go/pull/16 and really likes what @chris-ramon suggests. I am not sure I like the name change from types to gqltypes. I always alias the import if I have to

import gt "github.com/chris-ramon/graphql-go/types"

IMHO it is hard to read gqltypes without camel casing and still bit long. Aliasing gives the choice for the user and this library can use the apt name.

About getters, I like to keep the struct field private and use method to access it. One reason, in go we always use single letter to note the receiver, so typically we may write like

func someFunc(i types.Interface) {
  name := i.Name()
  resolveFn := i.ResolveFn()
}

It also depends on how frequently we use it though. In the todomvc-relay-go, I couldn't see any usage of accessing Name or ResolveFn field. But, I could see I use the Name field often once I split the definition into multiple packages. Still, n.Name() is better than to worry about 2 different types and to use node.GetName() or node.Name. Simplicity is one of the benefit of using interfaces as much possible.

Also in the main package, do we really need to use the acronym gql. I think it is better to be graphql. Not sure we need to use the name Graphql for the server, but could be Serve like graphql.Serve

Similarly gqlrelay could be just relay

not so sure about gqlhandler :-) I guess naming is the hardest problem :-)

sogko · 2015-10-16T01:55:28Z

I am not sure I like the name change from types to gqltypes

One of the proposals is to bring types/gqltypes into gql so that

import (
  "github.com/chris-ramon/graphql-go"
)
blogSchema, err := gql.NewSchema(&gql.Schema{
    Query: gql.NewObjectType(&gql.ObjectType{
       ...
    }),
})

What do you think about that direction?

Also in the main package, do we really need to use the acronym gql. I think it is better to be graphql. Not sure we need to use the name Graphql for the server, but could be Serve like graphql.Serve

I also think graphql as the package name would be cooler and great for branding/adoption. 😉
We might need more voice in this matter to see which way is the best to go 👍🏻

Similarly gqlrelay could be just relay
not so sure about gqlhandler :-) I guess naming is the hardest problem :-)

I wrote both graphql-relay-go and graphql-go-handler as offshoot packages and I based their package names on graphql-go and am not attached to their names either lol.
I would like to see a consistent package naming convention for (at least the core) golang+graphql ecosystem.

I did at one point of time wondered if it would make better sense for gqlhandler to be part of the graphql-go package, so that it could be used as

handler := gql.NewHandler()
or 
handler := graphql.NewHandler()

But then again, apart from the naming, currently there are parallels with GraphQL's Javascript ecosystem:

graphlql-go         ---->   graphql-js
graphql-relay-go    ---->   graphql-relay-js
graphql-go-handler  ---->   express-graphql

Anyone else want to share their thoughts in this? Would love to hear more voice and opinions 👍🏻

/cc @chris-ramon

chris-ramon · 2015-10-20T05:39:21Z

Great conversation about better pkg naming within the lib! really appreciate your thoughts 🌟

Currently we have graphql-go/types package with 11 files - #16 PR was an attempt to have a "better" package name for it, but gqltypes is hard to read and still suffers of stuttering, I agree with @bsr203 on perhaps naming it gt instead.

Also is great timing to share thoughts about have a better library API for graphql-go users - being one of the most important use cases: writing their GraphQL schema.

As @sogko pointed out, Solution #1 would be to bring graphql-go/types files to graphql-go/ which means that we will also need to move it's dependencies:

(graphql-go) -> $GOPATH/bin/deplist github.com/chris-ramon/graphql-go/types
github.com/chris-ramon/graphql-go/language/visitor
github.com/chris-ramon/graphql-go/errors
github.com/chris-ramon/graphql-go/language/ast
github.com/chris-ramon/graphql-go/language/kinds
github.com/chris-ramon/graphql-go/language/source
github.com/chris-ramon/graphql-go/language/location
github.com/chris-ramon/graphql-go/language/printer

Used this tool to list pkg dependencies.

Meaning all the files from those packages should be move to graphql-go/ so all files belongs to gql package, we need to do this to avoid loop imports.

After doing that plus fixing the current GraphQL stuttering and eventually updating struct instantiators NewYYY to receive pointers - we would end-up with a clearer API like:

import (
  "github.com/chris-ramon/graphql-go"
)
blogSchema, err := gql.NewSchema(&gql.Schema{
    Query: gql.NewObjectType(&gql.ObjectType{
       ...
    }),
})

Being the current API:

import (
  "github.com/chris-ramon/graphql-go"
  "github.com/chris-ramon/graphql-go/types"
)
blogSchema, err := types.NewGraphQLSchema(types.GraphQLSchemaConfig{
    Query: types.NewGraphQLObjectType(types.GraphQLObjectTypeConfig{
       ...
    }),
})

Said that, Solution #2 would be renaming graphql-go/types -> graphql-go/gt so we end-up with:

import (
  "github.com/chris-ramon/graphql-go"
  "github.com/chris-ramon/graphql-go/gt"
)
blogSchema, err := gt.NewSchema(&gt.Schema{
    Query: gt.NewObjectType(&gt.ObjectType{
       ...
    }),
})

lenaten · 2015-10-20T07:59:44Z

What about this convention:

follow the golang convention, not the js one.
every graphql type sit under graphql package (like solution 1)
omit the "graphql" string from every type name
maybe omit also the "type" string, since it's obvious.
maybe omit also the "config" structs (such GraphQLFieldConfig), it's make the constructor too verbose.
use a constructor functions (NewSomething()) instead of a composite literals, for better consistent API.

import (
  "github.com/chris-ramon/graphql-go"
)

query := graphql.NewObject(...)
mutation := graphql.NewObject(...)

schema, err := graphql.NewSchema(query, mutation)

bsr203 · 2015-10-20T09:57:01Z

@chris-ramon I too like everything to be under "graphql-go". I was saying, if one doesn't like like the name, they could alias it, but definitely like "gql". Thanks for the giving a momentum to this.

chris-ramon · 2015-10-26T20:25:03Z

Thanks a lot for sharing ur thoughts! - really appreciate it! 🌟

I would like it to summarize this issue to the following todos, if you have any final suggestions please do share.

Rename library name from graphql-go to graphql
Rename gql pkg name to graphql
Move types pkg files and it's dependencies tographql pkg
Remove GraphQL prefix and Type suffix from structs, interfaces and function constructors
Rename GraphQLType methods from GetName(), GetDescription(), GetError to Name(), Description(), Error(), add name, description and err private fields to structs that implements GraphQLType.
Remove Map suffix from structs, types and function constructors

lenaten · 2015-10-26T21:03:02Z

Very good summary!

In a second thought, the Type suffix should stay, since it's an ID of graphql type objects rather then other objects.

bsr203 · 2015-10-27T01:40:55Z

+1 for the todos . Thanks for your effort.

chris-ramon · 2015-10-27T03:03:16Z

Very good summary!

In a second thought, the Type suffix should stay, since it's an ID of graphql type objects rather then other objects.

Thanks for sharing ur thoughts on this @lenaten, I think we should remove Type to avoid stuttering and have cleaner API, eg:

Interfaces:

TypeDefinition -> Definition
GraphQLType -> Type

Structs:

GraphQLScalarType -> Scalar
GraphQLInputObjectType -> InputObject
GraphQLNonNull -> NonNull

Function constructors:

func NewNamedType -> func NewNamed
func NewGraphQLSchema -> func NewSchema
func NewObjectTypeDefinition -> func NewObjectDefinition
func NewGraphQLFormattedError -> func NewErrFormatted

Special cases*:

GraphQLInterfaceType -> Interface
GraphQLObjectType -> Object
ObjectTypeDefinition -> ObjectDefinition
InterfaceTypeDefinition -> InterfaceDefinition
ast.Type -> ASTType
GQLFRParams -> ResolveParams
GQLFormattedErrorSlice -> ErrFormattedSlice
GraphQLFormattedError -> ErrFormatted

For special cases* I think we should prefer rename to the shortest name the ones that will be use by lib users to build their graphql schemas, eg:

UnionTypeDefinition -> UnionDefinition instead of Union
GraphQLUnionType -> Union

lenaten · 2015-10-27T11:40:16Z

OK, let's do it!

sogko · 2015-11-03T02:31:46Z

Updates on this issue:

PR Fix Naming Issues #30 has been merged to main branch, thanks to @lenaten 👍🏻. The PR addressed most of the TODOs listed by @chris-ramon
Remaining TODOs are:
- GQLFRParams -> ResolveParams
- GQLFormattedErrorSlice -> ErrFormattedSlice
- GraphQLFormattedError -> ErrFormatted
  - I think the convention is for custom error structs is *Error (for e.g. PathError) and error variables is var err*
- Rename GraphQLType methods from GetName(), GetDescription(), GetError to Name(), Description(), Error(), add name, description and err private fields to structs that implements GraphQLType.
- Remove Map suffix from structs, types and function constructors
I've listed the most basic GraphQL types and compared them against graphql-js type system for reference: https://github.com/sogko/graphql-go/wiki/Type-System

Cheers!

chris-ramon · 2015-11-03T02:56:42Z

Thanks for the status update on Naming progress @sogko !

I went ahead and rename the library from graphql-go to graphql, so it matches the package name, this PR updates the README.

sogko · 2015-11-03T03:29:13Z

Great! Are you going to replace the import paths to github.com/chris-ramon/graphql too?

chris-ramon · 2015-11-03T03:32:00Z

Great! Are you going to replace the import paths to github.com/chris-ramon/graphql too?

Yup!, I'll submit a PR in a bit.

chris-ramon · 2015-11-03T03:49:32Z

Great! Are you going to replace the import paths to github.com/chris-ramon/graphql too?
Yup!, I'll submit a PR in a bit.

Import paths replaced on this PR.

bsr203 · 2015-11-06T16:17:03Z

hi.. shall we close this, and track the minor ones separate like #64

there are few more under Special cases section written by @chris-ramon . not sure what all donr already as I am just beginning to use the it after a pause.

Special cases*:

GraphQLInterfaceType -> Interface
GraphQLObjectType -> Object
ObjectTypeDefinition -> ObjectDefinition
InterfaceTypeDefinition -> InterfaceDefinition
ast.Type -> ASTType
GQLFRParams -> ResolveParams
GQLFormattedErrorSlice -> ErrFormattedSlice
GraphQLFormattedError -> ErrFormatted
For special cases* I think we should prefer rename to the shortest name the ones that will be use by lib users to build their graphql schemas, eg:

UnionTypeDefinition -> UnionDefinition instead of Union
GraphQLUnionType -> Union

bbuck · 2016-01-09T01:50:53Z

I think the general rename went well, but one thing I don't understand is why some things adopted certain naming patterns but others didn't.

For example, we now have Fields and Field but for arguments it's FieldConfigArgument which is actually a map... I can understand why it couldn't be Arguments but it could be FieldArguments and FieldArgument to match the naming patterns and still identify as an argument type for a field.

While I generally think the rename has gone really well thus far I think there is still places that can be approved on.

Evaluate list fields in parallel

… from here for perf-related PRs. - simple hello world equivalent - with a list - deeper query with list - query with many root fields and list in each of them - deeper query with many root fields and lists To really bring out probable issues with query perf, a latency is introduced to the data retrieval (previously it was just in-memory data fetch) When testing against branch before PR #20 and #21, this really highlighted the problem with evaluating list fields. In the future, more benchmarks for queries with fragments probably would be useful.

chris-ramon mentioned this issue Oct 21, 2015

Create basic example follow http://graphql.org/docs/getting-started/ #21

Merged

chris-ramon mentioned this issue Oct 27, 2015

Renames package types to gqltypes #16

Closed

lenaten mentioned this issue Oct 28, 2015

Fix Naming Issues #30

Merged

sogko mentioned this issue Oct 28, 2015

Rally community... #29

Closed

chris-ramon mentioned this issue Nov 2, 2015

Update README.md #34

Closed

sogko added the meta label Nov 6, 2015

FugiTech mentioned this issue Nov 8, 2015

Renames #68

Merged

sogko added a commit that referenced this issue Jun 11, 2016

Merge pull request #20 from andreas/experiment-parallel-resolve

6414283

Evaluate list fields in parallel

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Naming #20

Naming #20

bsr203 commented Oct 11, 2015

sogko commented Oct 12, 2015

bsr203 commented Oct 12, 2015

sogko commented Oct 16, 2015

chris-ramon commented Oct 20, 2015

lenaten commented Oct 20, 2015

bsr203 commented Oct 20, 2015

chris-ramon commented Oct 26, 2015

lenaten commented Oct 26, 2015

bsr203 commented Oct 27, 2015

chris-ramon commented Oct 27, 2015

lenaten commented Oct 27, 2015

sogko commented Nov 3, 2015

chris-ramon commented Nov 3, 2015

sogko commented Nov 3, 2015

chris-ramon commented Nov 3, 2015

chris-ramon commented Nov 3, 2015

bsr203 commented Nov 6, 2015

bbuck commented Jan 9, 2016

Naming #20

Naming #20

Comments

bsr203 commented Oct 11, 2015

sogko commented Oct 12, 2015

Struct names for GraphQL types

Getters method names

What we have right now

Considering an alternate GraphQLType interface

bsr203 commented Oct 12, 2015

sogko commented Oct 16, 2015

chris-ramon commented Oct 20, 2015

lenaten commented Oct 20, 2015

bsr203 commented Oct 20, 2015

chris-ramon commented Oct 26, 2015

lenaten commented Oct 26, 2015

bsr203 commented Oct 27, 2015

chris-ramon commented Oct 27, 2015

lenaten commented Oct 27, 2015

sogko commented Nov 3, 2015

chris-ramon commented Nov 3, 2015

sogko commented Nov 3, 2015

chris-ramon commented Nov 3, 2015

chris-ramon commented Nov 3, 2015

bsr203 commented Nov 6, 2015

bbuck commented Jan 9, 2016

Considering an alternate `GraphQLType` interface