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

Getters don't follow Go conventions #2359

Open
seth-r-thompson opened this issue Sep 8, 2022 · 6 comments
Open

Getters don't follow Go conventions #2359

seth-r-thompson opened this issue Sep 8, 2022 · 6 comments

Comments

@seth-r-thompson
Copy link

seth-r-thompson commented Sep 8, 2022

#1469 was closed by #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 #2331 (comment) 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
Copy link

droslean commented Sep 9, 2022

Same here. Is there any good approach?

@droslean
Copy link

droslean commented Sep 9, 2022

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

@seth-r-thompson
Copy link
Author

@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
Copy link

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
Copy link
Contributor

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
Copy link

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

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

No branches or pull requests

5 participants