google / starlark-go

Starlark in Go: the Starlark configuration language, implemented in Go
BSD 3-Clause "New" or "Revised" License
2.26k stars 204 forks source link

lib/proto: Allow replacing repeated fields, instead of appending. #440

Closed WojciechP closed 1 year ago

WojciechP commented 1 year ago

Consider the following Stralark line:

    msg.repeated = [1,2,3]

Without this commit, that line appends three elements to the previous existing repeated values. With this commit, that line replaces the previous repeated values with new three items.

I believe the old semantics is simply a bug, and that most users would expect an assignment to overwrite the previous value.

adonovan commented 1 year ago

Thanks for pointing out this bug, and for the fix.

I realize it would be cheeky of me to suggest that a CL like this might benefit from a test, but if you plan on investing in this package it would be worth your time to build one sooner rather than later.

WojciechP commented 1 year ago

I realize it would be cheeky of me to suggest that a CL like this might benefit from a test, but if you plan on investing in this package it would be worth your time to build one sooner rather than later.

Asking for tests is never cheeky :)

I added a small test for this, although there is number of choices to make, and I tried to choose reasonably:

I agree that if we are to make more use of the lib/proto module, it deserves a more comprehensive test suite. For the time being I think this is a good start to accompany the one-line fix.