99designs / gqlgen

go generate based graphql server library
https://gqlgen.com
MIT License
9.94k stars 1.16k forks source link

Getters don't follow Go conventions #2359

Open seth-r-thompson opened 2 years ago

seth-r-thompson commented 2 years ago

https://github.com/99designs/gqlgen/issues/1469 was closed by https://github.com/99designs/gqlgen/pull/2314, but while the implementation is a good step forward I don't think it utilizes golang to its fullest extent.

Following the change above, a schema defined as...

interface Resource {
  id: ID!
  title: String!
}

is generated as...

type Resource interface {
    IsResource()
    GetID() string
    GetTitle() string
}

However, this has two problems I see:

  1. Getters shouldn't have a Get prefix (see here)
  2. Methods should leverage go's multiple return values to return errors whenever applicable (see here)

It'd be better if the go type was generated as

type Resource interface {
    IsResource()
    ID() (string, error)
    Title() (string, error)
}

Naming convention is minor (but good practice), but the errors are more important IMO. For example as https://github.com/99designs/gqlgen/issues/2331#issuecomment-1221510135 points out there are many situations (eg. data loaders) where you can't get the value for some reason, and in the current implementation you'd either need to not report the error (bad) or panic (really bad). For methods that don't use a loader or have no reason to report any error, they can simply always return nil.

droslean commented 2 years ago

Same here. Is there any good approach?

droslean commented 2 years ago

@vektah This is a big blocker. Can you let us know if there is any workaround?

seth-r-thompson commented 2 years ago

@droslean In our repositories, we've pinned the library to a later commit that includes a flag to omit generating getters. This at least allows us to continue development.

dehypnosis commented 2 years ago

I think this is a breaking change..... what do you think about set omit_getters default value as false for backward compatibility. either increase major version?

neptoess commented 2 years ago

The Get prefix is a bit of a necessary evil. The field name can’t be the same as the getter name.

As for returning an error value as well, I personally that’s overkill for a getter (and even most setters), but it’s still a valid complaint. If your code has a small number of interfaces / types that need this, manually defining them is probably the best approach. If it’s more than that, I suggest disabling getter generation.

iyinoluwaayoola commented 1 year ago

I wouldn't return error for the getters, this will make it difficult to access nested fields. Rather than adding getters only for the interface, I'll find it very useful to also add them to the types (structs) that are used as pointers. This allows natural chaining of the method calls without worrying about runtime panics due to nil pointers.

See playground See relevant stackoverflow