Closed JD-Howard closed 2 years ago
@Sen-real Sorry this took so long to get out. Most of it was done a while ago, but my CompTIA Network+ turned into a nightmare of acronyms and magic numbers that required all of my attention. Glad I did it, but quite the memory exercise...
Also note, I need to investigate the change associated with 500dcb77ed3af0fc2764ad4746fe9c4f49b21292 and see if caused any issues. However, this is an issue related to our target version being 1.50 and me changing it to 1.57. So, if there is a problem, then it was already there and we just weren't aware of it. I assume it will be fine, simply because the AutoCompletion has pretty good test coverage these days, but I'll check on it tomorrow.
I have confirmed that 500dcb7 did not cause any issues. VSCode added an alternate data type to their interface, but we are assembling the concrete class we use with a string. So, the added toString()
was just a formality to downgrade the new interface feature to the primitive we were already providing.
I also confirmed #154 will be resolved by this PR.
Great work, thanks!
Objective
This is mostly an update of
DefinitionProvider
to use the infrastructure put in place by theRenameProvider
, but it also adds aReferenceProvider
that will essentially provide some visibility into the scope of theRenameProvider
.Abstractions
The DefinitionProvider was previously discussed to be a little squirrely on issue #88. Also hoping this will resolve #154 since I did test the F12 functionality on my Mac. The new "behavior" of the F12 now supports the
@Global
comment tags as its highest priority; which means the new version can support global variables from other documents that the original DefinitionProvider intentionally omitted. This was made possible by the infrastructure added in the RenameProvider PR. Also note that the new DefinitionProvider is very focused on finding the definition, the old one made attempts to "walk" up a document, but now that only happens if it is a non@Global
& non localized variable reference. IE, when there is no clear source.General Changes:
Tests performed
I believe everything related to this PR has at least 97% test coverage with the exception of what was added to the commands.ts since I can't think of a good way to test those entry points. However, they shouldn't really matter since they defer almost everything to the testable code.
Screen shot
The mock workspace I setup during the rename provider doesn't do anything to illustrate multiple potential references so I'll have to explain that behavior. The current F12 functionality will prefer an
@Global
marked function, but if there is no globally marked function and multiple identically named defun's exist, then it would present a list of possible definitions just like it did in the original implementation.https://user-images.githubusercontent.com/51800232/134130230-09bc306b-9bdb-4eab-8253-d09f12d9e893.mp4