-
Notifications
You must be signed in to change notification settings - Fork 742
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
Add type descriptions to completions and hovers, support method hovers. #1915
Conversation
8f5a00e
to
875ab9b
Compare
d01ee34
to
bb7f937
Compare
bb7f937
to
77dd44c
Compare
9ce98a5
to
6d4a78a
Compare
6d4a78a
to
a018cd7
Compare
...ore.Samples/Files/InvalidResources_CRLF/Completions/autoScalerPropertiesRequireEscaping.json
Show resolved
Hide resolved
src/Bicep.Core.Samples/Files/InvalidExpressions_LF/main.diagnostics.bicep
Outdated
Show resolved
Hide resolved
[] |
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 seems odd. Any idea why the completion items are removed?
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.
Good catch. I think because of #1915 (comment) - we use the declared type rather than assigned type for member access completions. I may need to revise that approach...
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.
Yeah...
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.
2440252 - here's is a more targeted fix I came up with to avoid the infinite recursion. I don't love it because it feels very special-case.
I did this because:
- language server functionality depends on the declared type for variable declarations being resolved (rather than just
any
) - we otherwise have infinite recursion (variable declared type -> variable value assigned type -> for loop assigned type -> object assigned type -> object declared type -> for loop declared type -> variable declared type)
Other potential fixes I thought of:
- removing the language server's dependency on declared type (big change, which I don't want to make with this PR).
- passing flags to
GetDeclaredType()
to shortcut recursion when called internally (by returningany
), but keeping the existing behavior when called externally.
Thoughts?
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.
Personally I prefer
removing the language server's dependency on declared type
to be the ultimate fix because that avoid special cases, but within the scope of this PR the fix looks good to me.
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.
Also I feel like this can be a question for Anders.
src/Bicep.Core.Samples/Files/InvalidVariables_LF/main.diagnostics.bicep
Outdated
Show resolved
Hide resolved
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.
Added some comments.
Btw, I'm assuming that find refs for properties is out of scope of this PR? |
Yes. I've added that as a note to the description. This implementation simply generates a new PropertySymbol when requested with no caching. Find refs would need a cache layer, plus means answering some tough questions about what we consider equivalent when comparing properties. |
Good find! I've fixed the hover and added a test for it. As you mention, showing a description is a little tricky. I think we have all the information we need in swagger, but we currently store descriptions in the individual property fields - meaning we only have descriptions for the sub class discriminator properties and not the base class discriminator property. I'll add an issue to the types repo for this. |
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 awesome!
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 great!
Seems like #1571 is unblocked, I’ll bring my branch up2date sometime next week. |
I was actually partway through merging main into your branch; I'll send you a PR if I get there, so you can choose whether or not you want to accept it. |
awesome! thanks :) |
@miqm I pushed accidentally rather than submitting a PR. Tests are passing though, which is a good sign! |
Closes #668
Closes #2074
NOTE: This PR does not include an implementation for find references on object property symbols.