-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Feat - extending decompilation to referenced queries #215
base: main
Are you sure you want to change the base?
Conversation
@magicmoux please rebase. |
{ | ||
IQueryable<int> query = Enumerable.Empty<int>().AsQueryable().Where(i => i >= 0); | ||
Test<Func<IQueryable<int>, IQueryable<int>>>( | ||
ints => Enumerable.Empty<int>().AsQueryable().Where(i => i >= 0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test is incorrect. It should be ints => query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is correct when based on the assumption that every nested IQueryable reference encountered should be also decompiled (I suppose that is the issue you raised by your comment that decompilation may go to far ?)
The test will fail anyway because of different outputs, but the difference would be acceptable (System.Int32[]
instead of the expected Empty().AsQueryable()
)
public void TestQueryableRefFromField() | ||
{ | ||
Test<Func<IQueryable<int>, IQueryable<int>>>( | ||
ints => Enumerable.Empty<int>().AsQueryable().Where(i => i >= 0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test is incorrect. It should be ints => fQref1.Where(i => i >= 0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see previous answer
public void TestQueryableRefFromStaticField() | ||
{ | ||
Test<Func<IQueryable<int>, IQueryable<int>>>( | ||
ints => Enumerable.Empty<int>().AsQueryable().Where(i => i >= 0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same also
+semver:feature Co-authored-by: Max <[email protected]>
I've fixed majority of the cases except EF tests. Also, it seems that decompilation here is going too far and maybe undesirable for some users. |
The nesting handling is so simple that I did not see it sitting right in front of me
I suppose you're talking about the fact that the solution provided here assumes that any |
@hazzik rebased and I restricted dereferencing queryables only to explicitly decompiled instances (also applied this to the EF tests) |
yes |
//ATTEMPT | ||
env.AboutToUseDelegateDecompiler(); | ||
|
||
var referencedQuery = env.Db.Set<Animal>().Where(it => it.Species == "Canis lupus").Decompile(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add test where the query would not be decompiled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @hazzik, got a bit sidetracked.
I'm not sure I understand what test you want added here, so I added one failing test to expose the issue and refactored the other to compare materialized results
@hazzik
here's PR #209 merged directly into OtpimzeExpressionVisitor.cs
Once again, I committed the unit tests first for demonstration.