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

Patcher and interfaces for custom types to be used in expressions as standard go types #487

Merged
merged 12 commits into from
Dec 26, 2023

Conversation

rrb3942
Copy link
Contributor

@rrb3942 rrb3942 commented Dec 1, 2023

See #460 for previous discussion.

I tried to cover all the types supported by expr, as well generic map[string]any and []any.

To take advantage of the interfaces you use value.Patcher as an option when compiling an expression.

patchers/value/bench_test.go Outdated Show resolved Hide resolved
patchers/value/bench_test.go Outdated Show resolved Hide resolved
patchers/value/value.go Outdated Show resolved Hide resolved
patchers/value/value.go Outdated Show resolved Hide resolved
patchers/value/value.go Outdated Show resolved Hide resolved

// Patcher is an expr.Option that both patches the program and adds the `getExprValue` function.
// Use it directly as an Option to expr.Compile()
var Patcher = func() expr.Option {
Copy link
Member

Choose a reason for hiding this comment

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

Let's use patcher pkg and rename Patcher to, for example, patcher.ValueGetter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should I define the actual expr.Function doing this? Should I move it somewhere else?

expr imports patcher so it creates an import cycle to create the function in patcher.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@antonmedv

Any advice on where I should define the expr.Function without causing an import cycle?

I don't think it should be defined as a normal builtin function?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I didn't understand the question.

I mean this: lets move code to patchers dir and rename this function to ValueGetter.

// Use it directly as an Option to expr.Compile()
var Patcher = func() expr.Option {
vPatcher := patcher{}
return func(c *conf.Config) {
Copy link
Member

Choose a reason for hiding this comment

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

We can reuse expr.Patch helper here.

func getExprValue(params ...any) (any, error) {

switch v := params[0].(type) {
case ExprValuer:
Copy link
Member

Choose a reason for hiding this comment

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

What about creating a separate function for each type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this be for performance reasons? I didn't want to pollute the expr function space with a bunch of functions.

I want to preserve the ability to implement both Any and a specific type interface. This lets you get compile type checks for the type, but still return nil at runtime. So, even if specific functions for each type were implemented, a type check would still need to be done. In some quick testing I had done earlier it seemed like as soon as you were paying the cost of type checking it didn't really matter the number of branches in the switch, or if you broke it into an if statement.

I can revisit if you would like.

patchers/value/value.go Outdated Show resolved Hide resolved
@@ -0,0 +1,260 @@
// Package value provides a Patcher that uses interfaces to allow custom types that can be represented as standard go values to be used more easily in expressions.
//
// # Example Usage
Copy link
Member

Choose a reason for hiding this comment

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

Let's add documentation about motivation and there this patcher can be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see 6be8f0e

Let me know if you would like anything additional.

@antonmedv antonmedv merged commit 500553e into expr-lang:master Dec 26, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants