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.
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
Resolve 1: Support for resolve in overloaded-record-dot #3658
Resolve 1: Support for resolve in overloaded-record-dot #3658
Changes from all commits
bb742b3
be71eb9
fb21134
f347ebc
c19480d
5e37f6f
4bcd45b
7d4f01e
225152e
4b34265
9985195
355e95c
2e4d14c
fb49c31
d1d299b
e025840
0b57d5a
735feca
6b3b915
1ba6098
7e9bf1d
0271ce2
794034b
7790755
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
WDYT about the idea of just reusing the
CodeActionParams
?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.
At least in this case, the CodeActionParams doesn't make sense. The problem with the codeActionParams is we are going to need to do processing anyways to know whether we can provide the codeAction, and right now it's a title too, so it makes sense to just process once instead of once to present and once to execute
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.
Right. I think it definitely makes sense that we'll want a "stateful version" of resolve-based handlers. Maybe we'll also want a stateless one... I think there are some plugins that don't define any of their own rules so don't even have anywhere to put state. But perhaps easier to do a few and then refactor afterwards.
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.
Just flagging that this seems like it's going to be a pretty much universal pattern for resolve handlers: the first thing they're going to do is decode their data from the data field. So we might want to make it part of the generic machinery.
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.
Looking at the spec, I'm not sure this is the correct meaning of
ContentModified
. I don't think there's actually anything wrong with the client sending us a document change notification in between asking for code actions and resolving them, even if we can't handle that. Perhaps justRequestFailed
?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.
It was mentioned in a GitHub issue that this was the way to do it. microsoft/language-server-protocol#1738
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.
Okay, let's link to that when we do this!
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.
Bonus points: make a PR upstream to clarify in the spec that this is what you should do!
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.
Is
newUnique
threadsafe? We run handlers in individual threads, so there could well be multiple instances of this running at once.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.
Yes https://stackoverflow.com/questions/70515566/when-is-data-unique-thread-safe
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.
great
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.
we might have to worry about contention on the ioref if we end up using this everywhere, but I expect that won't manifest until we're using it a lot. We should remember to try and check when we're nearly done