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

Parameter Evaluation in Ternaries Not Short Circuited #53

Closed
bkrn opened this issue Mar 30, 2017 · 4 comments
Closed

Parameter Evaluation in Ternaries Not Short Circuited #53

bkrn opened this issue Mar 30, 2017 · 4 comments
Assignees

Comments

@bkrn
Copy link

bkrn commented Mar 30, 2017

I would expect the following code to print 42 instead it fails for not finding bar.

ee, _ := govaluate.NewEvaluableExpression(`true ? [foo] : [bar]`)
fmt.Println(ee.Evaluate(
    map[string]interface{}{"foo": "42"},
))

There's arguments for that behavior but on the basis of issue #40 saying

Some users also specify a late-binding Parameters structure so that they don't need to pre-compute expensive variables every time

I had assumed that the Get method of the parameter interface was called only where needed (though I understand short circuiting logical AND/OR is an open issue). I'm in a place where I have some very expensive variables and would appreciate clarity on whether I'm missing something, found an issue, or looking at forking/going with another solution for evaluation.

Thanks for the awesome package.

@Knetic
Copy link
Owner

Knetic commented Mar 30, 2017

You're not missing anything, that's the expected behavior right now. There's no short-circuiting at all, the library will always Get both [foo] and [bar] during evaluation. The plan is to make the library's behavior match your expectation, but that needs #36 to happen. #40 got nixed because it would conflict with that (quite reasonable) expectation.

I don't know when #36 will happen, I haven't looked at it in some time. But I can give it a look sometime tonight and see if it's quick to do.

@Knetic Knetic self-assigned this Mar 30, 2017
@bkrn
Copy link
Author

bkrn commented Mar 30, 2017

For my purposes putting a check on evaluating the right side of a stage like

func evaluateTernaryRight(stage *evaluationStage, left interface{}) bool {
	if stage.symbol == TERNARY_TRUE {
		return left.(bool)
	} else if stage.symbol == TERNARY_FALSE {
		return left == nil
	}
	return true
}

//stuff happens and we're suddenly in evaluateStage

if stage.rightStage != nil && evaluateTernaryRight(stage, left) {
	right, err = this.evaluateStage(stage.rightStage, parameters)
	if err != nil {
		return nil, err
	}
}

seems to work well at first blush, expanding to && and || doesn't seem like an obstacle but I'll confess to not thinking about it much after ternaries worked

@Knetic
Copy link
Owner

Knetic commented May 1, 2017

That was a good starting point. I ended up being able to fully skip type-checking and calling another method after staring at it long enough. The library should now support short-circuiting all the operators that can be short-circuited. If you try master branch again your use case ought to work.

As of d534c6b (bulk of the work was done in 9f400ba)

@bkrn
Copy link
Author

bkrn commented May 1, 2017

Fantastic.

@bkrn bkrn closed this as completed May 1, 2017
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

No branches or pull requests

2 participants