-
-
Notifications
You must be signed in to change notification settings - Fork 561
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
Add test for eval.TooManyArgError() #3520
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.
This is great! I left a couple of comments that might be good to address, thank you!
@@ -99,6 +99,7 @@ func ResultType(identifier string, args ...any) *expr.ResultTypeExpr { | |||
} | |||
if len(args) > 2 { | |||
eval.TooManyArgError() | |||
return nil |
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 value returned by this function might be used to do validations later on so the idea is to not return nil as the validation code assumes there's always an expression (which could be invalid). So we shouldn't return here - or return a zero result expression.
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.
There are already two lines that return nil in ResultType. Are these wrong?
Lines 65 to 69 in 78b71ce
func ResultType(identifier string, args ...any) *expr.ResultTypeExpr { | |
if _, ok := eval.Current().(eval.TopExpr); !ok { | |
eval.IncompatibleDSL() | |
return nil | |
} |
Lines 109 to 116 in 78b71ce
for _, rt := range expr.Root.ResultTypes { | |
if re := rt.(*expr.ResultTypeExpr); re.Identifier == canonicalID { | |
eval.ReportError( | |
"result type %#v with canonical identifier %#v is defined twice", | |
identifier, canonicalID) | |
return nil | |
} | |
} |
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.
Good point, looks like Type
can also return nil
so this case is probably safe.
dsl/server.go
Outdated
@@ -59,6 +59,7 @@ import ( | |||
func Server(name string, fn ...func()) *expr.ServerExpr { | |||
if len(fn) > 1 { | |||
eval.TooManyArgError() | |||
return nil |
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.
same as above
Related to #3512