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

Made parameters an interface #14

Merged
merged 2 commits into from
Jun 29, 2016
Merged

Made parameters an interface #14

merged 2 commits into from
Jun 29, 2016

Conversation

oxtoacart
Copy link
Contributor

@Knetic Thanks for govaluate, I've found it to be very useful. One problem I had is that specifying parameters as a map[string]interface{} is a little inconvenient because I have them in a different data structure. I can convert by copying them, but that gets expensive for what I'm doing.

This PR provides a new Eval method that's like the existing Evaluate except that it takes a new interface Parameters. This allows me to pass in a different data structure that just provides a Get method.

var err error

cleanedParameters, err = sanitizeParamters(parameters)
cleanedParameters = &SanitizedParameters{parameters}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of sanitizing parameters up-front, we just wrap the supplied parameters with SanitizedParameters that does the validation at parameter access time.

@Knetic Knetic self-assigned this Jun 29, 2016
@Knetic
Copy link
Owner

Knetic commented Jun 29, 2016

I like these changes. Even if a user's structure doesn't have a method that exactly matches the signature of Get(string)(interface{}, error), this gives them a way to wrap their structures (like how MapParameters does) so that it works. This seems like a reasonable way to offer users with other data structures a way around copying.

I'll follow up with a couple minor edits for perf reasons (I noted this increased benchmark times by a flat ~50ns, which isn't huge, but is avoidable), but this looks good to me. Thanks for the PR! I'm glad the library is useful to you!

@Knetic Knetic merged commit 26ce45d into Knetic:master Jun 29, 2016
@oxtoacart
Copy link
Contributor Author

Thanks @Knetic !

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