-
Notifications
You must be signed in to change notification settings - Fork 53
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?
Conversation
Co-authored-by: TwitchBronBron <2544493+TwitchBronBron@users.noreply.github.com>
TwitchBronBron
left a comment
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.
Not quite. I've left some comments. Please make sure to update the tests to verify you're seeing the shotDescription or documentation explanations from the global callables in the hovers as well.
| `); | ||
|
|
||
| // Don't validate - this simulates a file that might not be in any scopes | ||
| const scopes = testProgram.getScopesForFile(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.
This is dead code. please remove. (you could find this out by running the lint rules before committing).
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 removing the unused variable. The build now passes without TypeScript errors.
| // After the fix, this should now work even when file has no scopes | ||
| expect(hover).to.exist; | ||
| expect(hover.range).to.eql(util.createRange(2, 29, 2, 32)); | ||
| expect(hover.contents).to.contain('function Abs(x as float) as float'); |
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.ts it shows the description for this function should be something like "Returns the absolute value of the argument.".
Here's what I'm expecting.
'a test function
sub test()
end sub
sub init()
test() 'hover over this, it includes "a test function"
end subBut 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.
Added verification for shortDescription and documentation in hover tests. The HoverProcessor now properly shows descriptions from globalCallables.ts for all global functions, both when files are in scopes and when they use the global scope fallback. |
Co-authored-by: TwitchBronBron <2544493+TwitchBronBron@users.noreply.github.com>
| 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'); | ||
| } | ||
|
|
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.
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)
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.
You're absolutely right. I've refactored the code to use a uniform approach for all callables. Instead of branching logic, I moved getTokenDocumentation to util.ts and modified BrsFile.ts to populate shortDescription when creating user-defined callables from comment tokens. Now HoverProcessor uses the same shortDescription/documentation properties for both global and user-defined functions uniformly. Commit b50df8d.
…cription from comment tokens Co-authored-by: TwitchBronBron <2544493+TwitchBronBron@users.noreply.github.com>
The hover functionality was not showing documentation for global functions in certain scenarios, while completions worked correctly. This was because the
getHover()method was missing the same fallback logic that exists ingetCompletions().Problem
When a file has no associated scopes (e.g., standalone files or files not yet validated), hovering over global functions like
Abs,Atn,Chr, etc. would return no hover information, even though these functions would appear in the completion list.Root Cause
The
Program.getHover()method usedgetScopesForFile(file)directly without a fallback to the global scope, whileProgram.getCompletions()had this fallback logic:Solution
Added the same global scope fallback logic to
getHover()that was already present ingetCompletions(). This ensures that global functions are always accessible for hover documentation, even when files don't belong to any specific scope.Testing
Abs,Atn,Cdbl, etc.)Fixes #476.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.