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

env[] keyword implemented in parser #382

Merged
merged 23 commits into from
Jul 12, 2023
Merged

env[] keyword implemented in parser #382

merged 23 commits into from
Jul 12, 2023

Conversation

mdmcconnell
Copy link
Contributor

@mdmcconnell mdmcconnell commented Jun 10, 2023

The env[] keyword allows an expression to access identifiers in env directly, with any name desired, including numbers, spaces, special characters, etc. Syntax env['string identifier'] and env["string identifier"] are allowable, as is any index that evaluates to a string.

@antonmedv
Copy link
Member

What about this case?

env[anotherVar]

@mdmcconnell
Copy link
Contributor Author

I think you are asking "what if the env[] index is an identifier, not a string?". Previously, we had dicussed along the lines of "env[] index must be a string", but allowing an identifier that evaluates to a string seems reasonable, if more complex. I will look at that.

@antonmedv
Copy link
Member

Also what about env[“foo” + “bar”]?

@mdmcconnell
Copy link
Contributor Author

That is another case of "something that evaluates to a string", versus "a string" which I had intended. env[strfun(stuff)] would be another example, etc. It's not the original idea, but I think a reasonable expectation. I will have to implement further "downstream" in expr execution.

@mdmcconnell
Copy link
Contributor Author

Back to this - have now implemented so that any index that evaluates to a string. I had to add a new opcode, since evaluation of the index may not happen until running the expression. This will not be backwarsd compatible with expressions that have a an array or map named "env" in their environment; I think that is common with expr keywords - I can not for example define a function named "len".

checker/checker.go Outdated Show resolved Hide resolved
checker/checker.go Show resolved Hide resolved
checker/checker.go Outdated Show resolved Hide resolved
compiler/compiler.go Outdated Show resolved Hide resolved
expr_test.go Outdated Show resolved Hide resolved
expr_test.go Outdated Show resolved Hide resolved
@antonmedv
Copy link
Member

Overall a good PR. Much simple implementation. 👍🏻

@antonmedv antonmedv merged commit 3c23d10 into expr-lang:master Jul 12, 2023
9 checks passed
@antonmedv
Copy link
Member

Nice work! 🎉

@antonmedv
Copy link
Member

Completely forgot. Please, write documentation for this feature. Thanks!

@mdmcconnell
Copy link
Contributor Author

Yes, of course - will write some documentation hopefully before next week. Thank you for your patience while I've been learning how expr package works!

@antonmedv
Copy link
Member

Also: will be nice to add this to expr/docgen

@harkli
Copy link

harkli commented Aug 7, 2023

The env[] keyword allows an expression to access identifiers in env directly, with any name desired, including numbers, spaces, special characters, etc. Syntax env['string identifier'] and env["string identifier"] are allowable, as is any index that evaluates to a string.

env["env"] will cause panic: attempt to misuse env keyword as env map key

func TestEnv(t *testing.T) {
env := map[string]interface{}{
"env": "demo",// panic!!!
}
p, e := expr.Compile(env == "demo", expr.Env(env))
o, e := expr.Run(p, env)
fmt.Println(o)
fmt.Println(e)
}

@mdmcconnell
Copy link
Contributor Author

That is WAD - it's not allowed to call any variable "env" (or other identifier) because the behaviour becomes indeterminate. Which should have precedence, the keyword or the user-defined variable? Should env[] recursively map into itself? There is some discussion above around 9-Jul. At the moment, other keywords like "len" can be overloaded, and there is intention to disallow that as well.

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.

None yet

3 participants