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
Make go-to-definition on a typeclass projection application go to the instance(s) #1767
Make go-to-definition on a typeclass projection application go to the instance(s) #1767
Changes from 19 commits
1c097ea
a0d6785
8c4cc19
c605ec5
cdd48eb
33e873a
4211619
e1a9ad8
ea1c12b
760d9b7
91ef175
5bf6e28
ca25434
b4b4f72
f2076aa
dc971fa
368c94b
7cf7ec1
cc9b38e
deb0bbb
9914b4c
9ebb657
a526a09
0c6534a
db476af
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.
Let's move this to
InfoUtils
and use it inhoverableInfoAt?
andlocationLinksOfInfo
etc. Thederiving TypeName
should probably be left here though.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.
Done, let me know what you think of the refactor.
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.
Hi, sorry to only comment on this now but
InfoWithCtx
is only really intended for use as a way of bundling data together in the RPC protocol so that interactive expressions and go-to-def in the infoview work. As Gabriel put it, it's a kind of "type popup + go-to-def thunk". Unless you need to do something with RPC here, it would be better not to move it and to add another type with whatever semantics you need instead.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.
Duplicating it wouldn't be hard, but since the type is opaque in the RPC protocol I'm not really seeing the motivation. It's a sensible bundle of data to have in the server in general, and the name fits as well. Even if we don't necessarily need
children
in the widgets.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.
The type seems like a better fit for
Elab.InfoTree
, if anything, because it was already hacky as far as the RPC interface is concerned — the outputs of delaboration areElab.Info
s but they should really be a much more restrictedDelab.Info
without variants like.ofCompletion
. But okay, sinceInfoWithCtx
is opaque we can always keep it around for uses such as in this PR but replace it with something else in the RPC interface. Btw @rish987 you probably want to also make the output ofhoverableInfoAt?
andtermGoalAt?
beInfoWithCtx
, document what thechildren
field is, and change the docstring to note that it's used for more than just RPC.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.
Done, thanks for the suggestions.