-
Notifications
You must be signed in to change notification settings - Fork 203
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
starlark: tweak how UnpackArgs enforces optional args #545
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
starlark/eval_test.go
Outdated
"x", &x, "y?", &y, "z", &z); err != nil { | ||
t.Errorf("UnpackArgs failed: %v", err) | ||
} | ||
if x != 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can achieve higher vertical density (and, in this case, readability) in both the code and the test log by combining all three in one line:
got := fmt.Sprint("x=%d y=%d z=%d", x, y, z)
want := "x=1 y=2 z=0"
...assert...
@@ -164,12 +163,15 @@ kwloop: | |||
} | |||
|
|||
// Check that all non-optional parameters are defined. | |||
// (We needn't check the first len(args).) | |||
for i := len(args); i < nparams; i++ { | |||
for i := 0; i < nparams; i++ { | |||
name := pairs[2*i].(string) | |||
if strings.HasSuffix(name, "?") { | |||
break // optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc comment says "If a parameter is marked optional, then all following parameters are implicitly optional whether or not they are marked." In hindsight we should probably have disallowed non-optionals after optionals, but that would be a breaking change now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
Fixes google#544 Signed-off-by: Nick Santos <nick.santos@docker.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Nick, thanks for sending this fix. LGTM.
Fixes #544