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.
Resolve 2: Support for resolve in hls-hlint-plugin #3679
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 2: Support for resolve in hls-hlint-plugin #3679
Changes from all commits
355e95c
fb49c31
985340b
9712ca5
26af22d
d1d299b
6b7754e
5335f1d
735feca
6b3b915
a57e5d9
d2ba87e
57119c4
9f9e807
68080fb
1ac3823
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.
What happens if it has the
command
set? It probably shouldn't, right? But maybe we should fail or at least log there.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.
Also, if it's cheap it would be nice to check that all the other fields of the code action are the same as the input one, i.e. the resolve handler only gave us an edit. In the more generalized case, we should check that the only fields that differ are the ones that the client says it can resolve. Then we can log if the handler does something wrong there.
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.
Well, I guess also the command and data fields should be unset?
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.
I think, according to the spec, we can only change the set of resolvable fields, so even though it would make sense to dump the data field, it still needs to stay the same as when we received it. (We also don't have the client capabilities so can't do anything with that). Finally in this case the only thing we can execute on is the returned WorkspaceEdit. We can return an error if any other field is changed, but not sure about the performance penalty of doing that.
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.
Can't we pass in the client caps here?
I don't know if it's worth doing the checking of field setting, it just seems like a way to sanity check plugins, which seems somewhat useful.
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, I guess we can't pass in the client capabilities here, but we should be able to pull them from the monad itself, since the plugin handler is effectively in the Lsp monad already
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.
Ah I see, this is exactly the sort of thing we could do if we had the client caps when making the handlers!
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 seems like we probably should care a little about the response? e.g. we probably want to do something if we fail, like at least log
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.
That's quite hard here, as we can't use a recorder (That's defined in the ghcide package, so cyclic dependency), and we only have a callback to work with.
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.
Ugh. Okay, I guess we can't do much here, but that's rather unsatisfying. Maybe we need to move some of the recorder stuff into
hls-plugin-api
as the "lowest" package.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.
I would really like to be able to do some logging in the generic functions you're defining, that way we can get somewhat-decent logging for lots of plugins by defining it in one place.