-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Revert "Merge pull request #63858 from CyrusNajmabadi/pullDiags" #65538
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
Revert "Merge pull request #63858 from CyrusNajmabadi/pullDiags" #65538
Conversation
|
I've marked it for servicing. @CyrusNajmabadi would you please fill in the ask mode and RCA in https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1684497? I would like to get this into 17.4, get a full validation build in VS and use it for some e2e testing between you, me and Manish. We can get a further pass from Richa's team if its necessary. I think what we have going for us here in taking the change is that we are just rolling back to a snap that was tested and known to be working for quite a while. But given the volume of the change, I expect questions/asks coming our way in terms of testing needed to mitigate risk. |
| // Live update squiggles have to be at least 1 character long. | ||
| var minimumLength = isLiveUpdate ? 1 : 0; | ||
| var adjustedSpan = AdjustSnapshotSpan(span, minimumLength); | ||
| if (adjustedSpan.Length == 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.
note: this restored logic means that push diags that have 0 length are filtered out. this is reflected in some test changes in this PR.
|
Checked in with Jason offline to confirm he has no additional concerns. Merging since we have soft approval. |
Fixes #65502
This reverts commit b2bbe29, reversing changes made to 78b0a64.
This moves us back to push-diagnostics for 17.4. The core issue issue we've run into is that push-diagnostics had granular push messages. So it could push syntactic diagnostics, then semantic diagnostics. This meant that syntax issues were reported (And cleared) quickly as the user typed, while semantic diagnostics could lag behind acceptably. While we will have that same mdoel for LSP pull diags, we switched to a form of pull-diags for squiggles without LSP that does not have that behavior. Specifically, it does all diagnostics at once, so syntax-diags are made/cleared at the same rate as semantic diagnostics, which makes things feel much slower.
So, for 17.4 we're simply reverting the change back to the safe code we know worked. For 17.5 we'll be back on pull, but with a fine-grained approach where we can do syntax and semantics independently.
We should NOT merge this branch into 17.5 (@ryzngard and @allisonchou fyi for revert of the auto forwarded revert in 17.5)
Tracked in https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1684497