-
Notifications
You must be signed in to change notification settings - Fork 31
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
[hover] Fix universe printing in hover. #839
Conversation
ad90b3f
to
293591f
Compare
How does the new version print? |
293591f
to
79d3065
Compare
controller/rq_hover.ml
Outdated
@@ -128,10 +162,11 @@ let pp_file fmt = function | |||
| Some file -> Format.fprintf fmt " - **in file**: `%s`" file | |||
|
|||
let pp_typ id = function | |||
| Def (typ, cr, file) -> | |||
| Def (typ, param, cr, file) -> |
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.
Probably best to call this sort_param
or something.
foo@{u u0 u1} (M : Type@{u}) : Type@{u0} -> Type@{max(u,u1+1)} |
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.
Seems like a fine patch for user experience, but the API could use a cleanup here and upstream.
Our code was incomplete. The API here could be really improved Coq-side. About is too verbose for hover, hence our code. We could opt for the version in `prettyp.ml` tho. Fixes #835
79d3065
to
0ae1e56
Compare
; full_path : Names.Constant.t option | ||
(** full path of the constant, if any, for example | ||
[Stdlib.Lists.map] *) | ||
; file : string option (** filename where the constant is located *) |
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.
Do we have access to the location too? (The one goto definition uses). Not relevant for this PR, but it might be useful to include.
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 could, but not sure line number is relevant here, the client can indeed issue the corresponding specific request for location.
Our code was incomplete. The API here could be really improved Coq-side. About is too verbose for hover, hence our code. We could opt for the version in
prettyp.ml
tho.Fixes #835