Add missing hover docs #204
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For non-shape keywords, I added a new shape to the builtins model, NonShapeKeywords. I also had to update Statements so you can check if the cursor is within the keyword because some statements don't need to store the keyword Ident. Then I updated IdlPosition to perform this check. I also added way to include some externalDocumentation links to the hover content of all members in a builtin shape, with the intent that members which don't have a doc link themselves will still have some link people can use to get to the Smithy site if they need more detail that isn't provided in the hover content. The way this works is, if the builtin shape you're getting the hover content for is a member, we also append the externalDocumentation defined on its container shape. This way we don't have to repeat the trait on every member.
Docs weren't being rendered for hover on members. Also, the builtin shape docs for list and map members could clobber the user-defined docs, so I removed them.
milesziemer
requested review from
JordonPhillips,
yefrig and
joewyz
and removed request for
JordonPhillips
yefrig
approved these changes
Feb 27, 2025
joewyz
approved these changes
Feb 27, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds hover info for a bunch of things that I didn't get around to in the initial hover upgrade,
including:
metadata,namespace,use,apply,with, andforI had to add new builtin model for the non-shape keywords, but reused
ShapeMemberTargetsforshape type keyword hover.
I also had to add a way to check if the cursor was inside the keyword of a statement, which simplifies
IdlPosition a bit too.
The Smithy docs links use
externalDocumentation, like the smithy-build.json model, and I also addeda way to include docs links to all members of a builtin shape, because there's some things which don't
have a great link for themselves specifically, but it would still be nice to allow navigating to some
docs page that could be useful. This works by adding the
externalDocumentationtrait to the rootshape in the builtins model, and having the hover implementation add any external docs from the
container shape when constructing hover for a builtin. So, for example, hovering over
"version": "1.0"in smithy-build.json now also includes a link to the smithy-build.json page.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.