display: only hide replacement steps in diff #16065
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When displaying diff events, we currently try to hide non-logical replacement steps unless we specifically enable showing them. However we currently do that for all non-logical operations, regardless whether they are replacement steps or not. In particular a RefreshStep is non-logical, but it's also not a replacement step. We still want to show them during the diff because their output can be important. Especially if the user just requested a diff it doesn't make sense to hide the diff from them at the same time. The intention here is to only hide replacement steps, so do that.
Changelog[uncommitted] (2024-04-26)Bug Fixes
|
tgummerer
force-pushed
the
tg/fix-missing-display-output
branch
from
c594528
to
efdb322
Compare
|
Yeh this looks more sensible |
Frassle
approved these changes
Apr 26, 2024
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When displaying diff events, we currently try to hide non-logical replacement steps unless we specifically enable showing them. However we currently do that for all non-logical operations, regardless whether they are replacement steps or not.
In particular a RefreshStep is non-logical, but it's also not a replacement step. We still want to show them during the diff because their output can be important. Especially if the user just requested a diff it doesn't make sense to hide the diff from them at the same time.
The intention here is to only hide replacement steps, so do that.
The full diff with the display tests is here: https://gist.github.com/tgummerer/fcd012f13669a9cdc39530cde7770260 It's unedited, so it includes some flakyness which isn't interesting.
I looked it over, and I think it looks like what we want, but I'm curious to hear what others think. E.g. https://gist.github.com/tgummerer/fcd012f13669a9cdc39530cde7770260#file-testdata-diff-L558 looks more correct now, as it shows the two delete operation that actually happened, that it didn't show before, and it still shows the same operation (Calling this one out in particular, since it took me a bit to understand that we still have the same operation in the diff)
Fixes #7665