-
Notifications
You must be signed in to change notification settings - Fork 26
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
Document symbol provider #27
Conversation
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.
This is super cool, I'm impressed how much monaco provides out of the box. I pushed some changes to your branch, including adding a more interesting corpus.
val pos = Position("???", symPos.start.offset, symPos.end.offset) | ||
new SymbolInformation( | ||
name = denotation.name, | ||
containerName = "???", |
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.
Try denotation.info
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.
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 can probably implement a utility to shorten those fully qualified names org.typelevel.paiges.Doc
into Doc
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 fully qualified names make sense to me especially for cross project metadoc sites.
val denotations = attrs.denotations.map { case (s, d) => s -> d }.toMap | ||
val symbols = for { | ||
(symPos, sym) <- attrs.names | ||
denotation <- denotations.get(sym) |
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 should probably filter out parameters, a symbol is any definition, including def parameters/class fields and bound names in pattern matching.
This seems to do an OK job
(!isPARAM && !isTypeParam) && {
isClass ||
isTrait ||
isObject ||
isDef ||
isVal
}
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.
Glad you pitched in on getting this working!
else if (denotation.isClass) | ||
Some(SymbolKind.Class) | ||
else if (denotation.isObject) | ||
Some(SymbolKind.Module) |
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.
.Object
) = { | ||
val denotations = attrs.denotations.map { case (s, d) => s -> d }.toMap | ||
val symbols = for { | ||
(symPos, sym) <- attrs.names |
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 we currently don't have the model's filename, model.uri
is inmemory://model/1
. Once we solve #19 then we can filter out the symbols by filename.
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.
Depending on your progress we can also start by creating the model separately which allows to define the URI.
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've been digging into the vscode api, and learning how to use desktop vscode at the same time 😅
Seems the ITextModelService is what we need to provide.
- Add more interesting corpus. - Use Index instead of Attributes.names. - Filter out parameters.
82f4031
to
5942b07
Compare
index.files | ||
.find(_.endsWith("Doc.scala")) | ||
.get | ||
.replace(".scala", ".semanticdb") |
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 should probably add an entrypoint
setting to the CLI and store it in metadoc.index for use here, unless we show some kind of directory browser on start.
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.
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 propose we fix that once we nail #19
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.
👍
Are we good to merge this? |
isVal | ||
} | ||
} | ||
kind <- symbolKind(denotation) |
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 are filtering twice here, since anything we cannot map to a SymbolKind won't be shown. I suggest we change the above filter to only remove parameters and type parameters.
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.
Although I think there's a SymbolKind for parameters.
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.
Nice catch, I will update.
Definitely, let's merge. It looks great. |
Refs #21
Will need some help on how to extract the symbol info from Scalameta's
Symbol
andDenotation
.