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

Issues 212, 256: Adding support for internal lambdas to reference parameters from the parent lambda #257

Merged
merged 3 commits into from
Sep 21, 2022

Conversation

holdenmai
Copy link
Contributor

@holdenmai holdenmai commented Sep 19, 2022

Issue #212 was really the issue here.

When replicating the code given in Issue #256 the last "x.Grade" was being immediately resolved as the value from the parameter was being inserted directly. In the previous implementation of "InterpreterExpression", it was creating it as a new ParameterExpression.
This code update captures the "parent" ParameterExpressions as identifiers.

This is essentially what happens with compiled code, so instead of us having to create a class, Expression actually allows us to reference the existing Expression as an Identifier.

Fix #212, #200, #256

@davideicardi
Copy link
Member

LGTM! Thank you!

@metoule do you see problems?

@davideicardi
Copy link
Member

Do you think that this PR will solve #212, #200 and #256?

@holdenmai
Copy link
Contributor Author

Yes it will.

This will also make it so that #213 can be closed without merging as the settings Identifier dictionary has become our "locals" dictionary.

I just thought of a couple more cases to verify that we are only able to access parameters defined in the enclosing lambda, not a parameter defined by an adjacent lambda.

Will get those included as soon as able.

@holdenmai
Copy link
Contributor Author

Test cases to cover duplicating parameter names in other lambdas have been added.

@davideicardi
Copy link
Member

Thank you @holdenmai . It looks like a very nice feature. There is just a failed test.

@holdenmai
Copy link
Contributor Author

Unit test has been fixed. It actually had a bad lambda declaration that would not have compiled with real code. I guess the only issue with that is if a consumer of this package had also done the same thing in the past.

Copy link
Contributor

@metoule metoule left a comment

Choose a reason for hiding this comment

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

That's even better than my attempt in #213 (and simpler :)), thanks a lot!!

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.

Parsing Lambda expression pre-evalutes it with given arguments
3 participants