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

feat: return zero value in getter methods if receiver is nil #359

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

WingT
Copy link

@WingT WingT commented Oct 21, 2024

Add nil check in go structs' getter methods, making chained function call like a.GetB().GetC().GetD() won't panic.

I have:

  • Written a clear PR title and description (above)
  • Signed the Khan Academy CLA
  • Added tests covering my changes, if applicable
  • Included a link to the issue fixed, if applicable
  • Included documentation, for new features
  • Added an entry to the changelog

Change-Id: If1c1c5f305cae0abda2f80b8c3e3c1ec37a3d8dc
@benjaminjkraft
Copy link
Collaborator

Hmm. My intuition is that this seems sketchy (why should (nil).GetId() return ""?), but I don't really have a clear idea of why (as far as I understand the only code that changes behavior is code that currently panics). Let me think on this some more, I need to remember some more of the reasoning around this.

Maybe one middle ground is to do this only if the return is also a pointer. That makes resp.GetA().GetB().GetC() work, but means we never construct empty strings, etc. Although even that has the property that it means there are situations where you can write resp.A and resp.GetA() and they have different behavior, which is perhaps not ideal. And as discussed in #301, it's a bit awkward.

BTW, part of the problem is that pointers are a bad way to do optionals in Go. (In some sense the true Go Way is to not use the pointer at all, and just get a giant empty struct for optionals, although I know that may have other drawbacks.) It might be interesting to experiment with using a generic Maybe[T] to do this. Then you can use the (functorial) Map to follow the optionals explicitly. Although that probably gets quite verbose in practice, because Go doesn't have succinct lambda syntax (nor unbound method syntax, etc.).

Anyway, let me think a bit more about this before we merge. It's a simple change in the input but it seems like a bigger one in the output.

@WingT
Copy link
Author

WingT commented Dec 10, 2024

I think this is a really useful feature, especially for queries with long nested fields.

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.

2 participants