Completion: fix qualified completion in sequence expressions #18111
+38
−6
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
❗ Release notes required
|
T-Gro
reviewed
Dec 6, 2024
| // It's a nested sequential expression. | ||
| // Visit it, but make defaultTraverse do nothing, | ||
| // since we're going to traverse its descendants ourselves. | ||
| yield dive expr expr.Range (fun expr -> visitor.VisitExpr(path, traverseSynExpr path, (fun _ -> None), expr)) |
There was a problem hiding this comment.
expr1 is "dive"'d right below this call, and expr2 is handled via the recursive invocation => cannot see any impact from ommiting dive into the expr itself.
=> LGTM.
There was a problem hiding this comment.
Does visitor.VisitExpr still get called on expr itself now though? Maybe it does and I'm just not awake yet :)
T-Gro
approved these changes
Dec 6, 2024
auduchinok
force-pushed
the
completion/seqExpr-qualifier
branch
2 times, most recently
from
28016cd to
6299e69
Compare
auduchinok
changed the title
auduchinok
force-pushed
the
completion/seqExpr-qualifier
branch
from
6299e69 to
886a003
Compare
auduchinok
force-pushed
the
completion/seqExpr-qualifier
branch
from
886a003 to
8487881
Compare
|
This has been green for a while, but the release notes constantly need resolving conflicts. |
|
Yeah release notes conflicts can be annoying, tho we sometimes resolve them to keep PRs in shape :) This LGTM, I was just also wondering about what Brian mentioned above. |
psfinaki
approved these changes
Dec 10, 2024
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.
Fixes #18033. I'm not entirely sure what the removed lines were intended to do, so maybe removing the reverts the fix introduced in #16882. The test added in #16882 passes for locally with this change.