Naming about graphql HOT 18 OPEN

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;

1. direct access and,
2. 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!

from graphql.

@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 :-)

from graphql.

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

from graphql.

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{
       ...
    }),
})

from graphql.

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)

from graphql.

@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.

from graphql.

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

from graphql.

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.

from graphql.

+1 for the todos . Thanks for your effort.

from graphql.

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

from graphql.

OK, let's do it!

from graphql.

Updates on this issue:

• PR #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!

from graphql.

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.

from graphql.

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

from graphql.

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.

from graphql.

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.

from graphql.

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

from graphql.

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.

from graphql.