Respect transforms in lazy eval#5447
Conversation
There was a problem hiding this comment.
Curious, why use Filename instead of Identity?
There was a problem hiding this comment.
I was copy/pasting from the real repro :) Identity does make more sense.
rainersigwald
force-pushed
the
respect-transforms-in-lazy-eval
branch
from
fbeea25 to
a031d2f
Compare
rainersigwald
force-pushed
the
respect-transforms-in-lazy-eval
branch
from
a031d2f to
2d5a47c
Compare
|
cc @mmitche who suggested getting this into Preview7 |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Forgind
left a comment
Forgind
left a comment
There was a problem hiding this comment.
I don't actually know what evaluating an int does, but since it got this far, it's presumably fine. LGTM.
There was a problem hiding this comment.
nit:
| <i Remove='@(i->`%(Extension)`)' /><!-- should do nothing -->"; | |
| <i Remove='@(i->`%(Extension)`)' /> <!-- should do nothing -->"; |
There was a problem hiding this comment.
nit:
| <i Update=`@(i->'%(Extension)')`><!-- should do nothing --> | |
| <i Update=`@(i->'%(Extension)')`> <!-- should do nothing --> |
There was a problem hiding this comment.
Should this always return false or should it occasionally return true? I think this is only used for performance, so it doesn't matter if it has false negatives now, but I'd like to see if I understand it correctly. Also could be relevant if we start using it elsewhere.
There was a problem hiding this comment.
It should always return false. The question this method answers is "can I just remove/update every item in the group, or do I need to expand the value to match against existing items?"
Renamed it for a bit more clarity there.
There was a problem hiding this comment.
Could we just move the assert up? Seems simpler.
There was a problem hiding this comment.
That was my initial implementation, but it loses a bunch of information in the error message--it's nicer to see the item-level diff if there is one. That helped me chase down a couple of errors in this implementation.
There was a problem hiding this comment.
| int minimumLength = Math.Min(expectedItems.Length, items.Count()); | |
| int minimumLength = Math.Min(expectedItems.Length, items.Count); |
These fired while I was writing a new test but didn't have much useful information. Keep AssertItems from throwing an ArgumentOutOfRangeException on mismatched lengths, and give a clue or two about mismatched lengths in AssertItemHasMetadata.
Ensure that Update and Remove operations done at evaluation time that use item functions pay attention to the item function and don't apply to all items of the same type.
Fixes dotnet#5445 by checking to see if an item function is invoked (the captured expression has subcaptures) before optimizing operations on same-item captures.
The question this method answers is 'can I just remove/update every item in the group, or do I need to expand the value to match against existing items?' Renamed it for a bit more clarity there.
rainersigwald
force-pushed
the
respect-transforms-in-lazy-eval
branch
from
2d5a47c to
0ffb5c4
Compare
5 participants
Fixes #5445 by checking to see if an item function is invoked (the captured
expression has subcaptures) before optimizing operations on same-item
captures.
Added regression tests and improved some asserts that didn't really help me when they fired.
fyi @cdmihai, @safern