-
Notifications
You must be signed in to change notification settings - Fork 523
Consistently pass $nativeExpressionTypes
in MutatingScope
#2021
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
Consistently pass $nativeExpressionTypes
in MutatingScope
#2021
Conversation
For these changes, there should definitely be some tests that fail without these changes. And one idea - we should move nativeExpressionTypes to be right after expressionTypes in MutatingScope constructor + in InternalScopeFactory. |
Right, I thought the same, but was struggling with tests. I just managed to come up with something and can hopefully push it soon.
OK, then I'll quickly create a PR for that, that simplifies my other PRs including this one 👍 |
Oh, and unfortunately I need #2017 first to make the regression tests here work |
0093e29
to
403844b
Compare
while adding some tests I realized that currently $thisType is passed into nativeExpressionTypes, which isn't correct. Do we want to retain and pass a $nativeThisType? it would definitely work, but duplicate some code basically. UPDATE: I think I dealt with it, also added tests for everything and ensured that they fail without the commit. they should turn magically green after merging #2017 and rebasing |
df7244f
to
ee8d630
Compare
This pull request has been marked as ready for review. |
This one should be rebased now I guess :) |
yes and no. it needs #2017 unfortunately :) but really, no worries! also, I need to fix conflicts now |
ee8d630
to
b7215c1
Compare
Thank you! |
because why not? no really, I have no clue and am asking myself exactly that.
I went through all ScopeFactory::create() calls and this makes the native expression types passing consistent with the expression types passing AFAIK.
This passes native types properly for
CC @rajyan