-
Notifications
You must be signed in to change notification settings - Fork 64
Fix hover for global functions by adding global scope fallback #1541
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
fc8c8bf
2465506
79ebcfd
b50df8d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import { isBrsFile, isFunctionType, isXmlFile } from '../../astUtils/reflection'; | ||
| import type { BrsFile } from '../../files/BrsFile'; | ||
| import type { XmlFile } from '../../files/XmlFile'; | ||
| import type { Hover, ProvideHoverEvent } from '../../interfaces'; | ||
| import type { Hover, ProvideHoverEvent, Callable } from '../../interfaces'; | ||
| import type { Token } from '../../lexer/Token'; | ||
| import { TokenKind } from '../../lexer/TokenKind'; | ||
| import { BrsTranspileState } from '../../parser/BrsTranspileState'; | ||
|
|
@@ -39,6 +39,32 @@ | |
| return parts.join('\n'); | ||
| } | ||
|
|
||
| private buildCallableContents(callable: Callable, fence: (code: string) => string) { | ||
| const parts = [fence(callable.type.toString())]; | ||
|
|
||
| // For global callables, use shortDescription and documentation properties | ||
| if (callable.shortDescription || callable.documentation) { | ||
| const docs = []; | ||
| if (callable.shortDescription) { | ||
| docs.push(callable.shortDescription); | ||
| } | ||
| if (callable.documentation) { | ||
| docs.push(callable.documentation); | ||
| } | ||
| if (docs.length > 0) { | ||
| parts.push('***', docs.join('\n\n')); | ||
| } | ||
| } else { | ||
| // For regular callables, use token-based documentation | ||
| const tokenDocs = this.getTokenDocumentation((this.event.file as BrsFile).parser.tokens, callable.functionStatement?.func?.functionType); | ||
| if (tokenDocs) { | ||
| parts.push('***', tokenDocs); | ||
| } | ||
| } | ||
|
|
||
| return parts.join('\n'); | ||
| } | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you have to write the code like this? Aren't the global callabales stored the same way as other functions? I'd rather have the documentation stored on the functions in a unoform way so that user-defined functions and global functions both expose their documentation in the same way. Go find where we expose the global callables and make sure their docs are included properly. Perhaps with comment tokens or something (just a thought, don't do this unless it's already being done this way elsewhere)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're absolutely right. I've refactored the code to use a uniform approach for all callables. Instead of branching logic, I moved |
||
| private getBrsFileHover(file: BrsFile): Hover | undefined { | ||
| const scope = this.event.scopes[0]; | ||
| const fence = (code: string) => util.mdFence(code, 'brightscript'); | ||
|
|
@@ -115,7 +141,7 @@ | |
| if (callable) { | ||
| return { | ||
| range: token.range, | ||
| contents: this.buildContentsWithDocs(fence(callable.type.toString()), callable.functionStatement?.func?.functionType) | ||
| contents: this.buildCallableContents(callable, fence) | ||
| }; | ||
| } | ||
| } | ||
|
|
||
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 need to also see the description of the function. Looking at
globalCallables.tsit shows the description for this function should be something like "Returns the absolute value of the argument.".Here's what I'm expecting.
But for the global functions, it doesn't. It's not even related to files with zero scopes. I can reproduce this in my source/main.brs 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.
Fixed by modifying HoverProcessor to show shortDescription and documentation from global callables. The hover now includes descriptions like "Returns the absolute value of the argument." for global functions. Updated tests to verify the descriptions are shown.