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

Should repeated and map proto fields by writable by default? #512

Open
negz opened this issue Oct 2, 2023 · 1 comment
Open

Should repeated and map proto fields by writable by default? #512

negz opened this issue Oct 2, 2023 · 1 comment

Comments

@negz
Copy link

negz commented Oct 2, 2023

I'm new to Starlark1. I'm attempting to write a little Go function that essentially defers answering a gRPC request to a Starlark script. Like this:

v1beta1 = proto.file("v1beta1/run_function.proto)

def main(req):
  # Do some stuff with req, which is a v1beta1.RunFunctionRequest message...

  return v1beta1.RunFunctionResponse(
    results=[v1beta1.Result(
      severity=v1beta1.Severity.SEVERITY_NORMAL,
      message="Hi from Starlark!"
    )]
  )

My first intuition was to try doing something like this2:

  rsp = v1beta1.RunFunctionResponse()
  rsp.results.append(v1beta1.Result(message="Hi!"))

However, I was surprised to be told that rsp.results was a frozen repeated field. Looking at proto.go I realized this was the default value, so I tried this:

  rsp = v1beta1.RunFunctionResponse()
  rsp.results = []
  rsp.results.append(v1beta1.Result(message="Hi!"))

Unfortunately the field is still frozen. I'm guessing because the Has() method of protoreflect.Message only considers a message to have a repeated field (and thus I guess by extension map field) if it's non-empty.

With all that said, I'm curious:

  • What's the reasoning behind default message field values being frozen?
  • Would you be open to a PR that made them writable?

I'm not sure it's possible to make them writable/unfrozen by default but I wanted to ask before I dug in too deeply.

Footnotes

  1. I guess technically I used it in a bunch of BUILD files a decade ago, but I've forgotten all about that. 🙂

  2. I added the append method in https://github.com/google/starlark-go/pull/511.

@negz
Copy link
Author

negz commented Oct 2, 2023

I think part of what I'm looking for, especially for maps (in the context of #511), is an API like this:

if msg.map_field == None:
  msg.map_field = {}
msg.map_field["key"] = "value"

This is similar to what was mentioned in stripe/skycfg#23 (comment). I can use not proto.has instead of == None easily enough, but I'll still end up with a frozen field because the msg.map_field = {} doesn't effectively replace the default value with a usable dict/MapField.

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

1 participant