Show documentation on hover #1025
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
Essentially this works now, except that the Anyway, here's a demo: hover.mp4Btw, why can't I hover on |
|
It seems like this needs rebasing on #930, @marvinborner. 👀 |
|
Yes, otherwise it's a bit hard to review :) Please ping me once it's rebased! |
marvinborner
force-pushed
the
feature/documentation-on-hover
branch
from
7fe591d to
a304fb2
Compare
|
@jiribenes @timsueberkrueb rebased! |
|
Another mysterious CI error in |
| documentation.flatten | ||
| .map('\n' +: _) | ||
| .getOrElse("") | ||
| .replace("\\n", "\n") |
There was a problem hiding this comment.
Is .flatten.map f the same as .flatMap f?
There was a problem hiding this comment.
It seems we have:
But then, why do we pass an Option[Doc] (see other comment)?
Also, maybe instead of .map(...).getOrElse(...), a match would be clearer?
There was a problem hiding this comment.
Sorry for the late response.
Why is this needed?
We only want to add a newline delimiter if the symbol actually has a doc comment, therefore we map on it and then .getOrElse. We need to unescape the newlines in order to show them in VS code as literal newlines (we build the string with escaped newlines in RecursiveDescent)
Is
.flatten.map fthe same as.flatMap f
No, as it turns out. Also, I have changed the types from Option[Doc] to Doc, so this is not required anymore (see other comment)
Also, maybe instead of .map(...).getOrElse(...), a match would be clearer?
It's of course a stylistic choice, but I think it is clear enough as is.
| case Some(p: Product) => | ||
| p.productElementNames.zip(p.productIterator) | ||
| .collectFirst { | ||
| case ("doc", Some(s: String)) => Some(s) |
There was a problem hiding this comment.
I wonder if there's a way to get rid of the magic string literal here somehow 🤔?
There was a problem hiding this comment.
Not sure, I am not aware of it. I also do not really have a problem with this implementation, I think it is precisely the solution which our way of annotating Docs requires us to use.
There was a problem hiding this comment.
Could the Trees that (potentially) have doc comments inherit from some Documented trait like:
trait Documented {
def doc: Doc
}
// ...
enum Def extends Definition with Documented {...}
// ...
case class Constructor(..., doc: Doc, ...) extends ... with Documented?
Then this could be a
case Some(d: Documented) => d.docThen we will probably get better compiler support.
Or am I misunderstanding something?
|
The test case did indeed reveal a problem in the parsing of doc comments, where the initial doc comment was always ignored because of empty/implicit Other than that, this should be mergeable now. |
Closes #1006, blocked by #930.