Skip to content

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

Merged
metoule merged 3 commits intodynamicexpresso:masterfrom
holdenmai:256_NestedLambdas
Sep 21, 2022
Merged

Issues 212, 256: Adding support for internal lambdas to reference parameters from the parent lambda#257
metoule merged 3 commits intodynamicexpresso:masterfrom
holdenmai:256_NestedLambdas

Conversation

@holdenmai
Copy link
Copy Markdown
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
Copy Markdown
Member

LGTM! Thank you!

@metoule do you see problems?

@davideicardi
Copy link
Copy Markdown
Member

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

@holdenmai
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@davideicardi
Copy link
Copy Markdown
Member

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

@holdenmai
Copy link
Copy Markdown
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
Copy Markdown
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