Improve UX of completion items #115
Conversation
|
I'm not sure why the unit test isn't passing, there is something happening in the mock server I think that isn't replicating the normal behavior, as I see the documentation data coming through in the UI. |
|
Without markdown support in the LS protocol, we may want to convert markdown to plain text in the near term, not sure. I'll open an issue for markdown support. |
There was a problem hiding this comment.
Overall I like these changes and I think it's almost ready for merge, besides that one failing test.
Without markdown support in the LS protocol, we may want to convert markdown to plain text in the near term, not sure. I'll open an issue for markdown support.
If the markdown format becomes the only format for descriptions then we'd definitely need a way of stripping off the markdown somehow, because markdown is opt-in by client per LSP, so we will always need to have a plaintext version for clients that don't support markdown.
It makes me think we probably should have implemented this as dual (plaintext/markdown) fields in the protocol, instead of 1 fields with another one specifying whether it's plaintext or markdown. I'm not sure how well we can turn markdown into plaintext if it contains anything else than inline formatting. Block-based formatting (like heading or ```) will certainly loose meaning and probably mess up something.
| requiredText := "Required" | ||
| if len(blockType.BlockList) >= int(blockS.MinItems) { | ||
| requiredText = "Optional" | ||
| detail := fmt.Sprintf("Block, %s", blockS.NestingMode) |
There was a problem hiding this comment.
I reckon we'll iterate on the human-readable text here over time to e.g. print list of blocks, single block etc. but this is already big step forward and I like these changes 👍 💯
There was a problem hiding this comment.
Agree, just wanted to move closer to core concepts.
internal/lsp/completion.go
Outdated
| @@ -34,6 +34,9 @@ func CompletionItem(candidate lang.CompletionCandidate, pos hcl.Pos, snippetSupp | |||
| Kind: lsp.CIKField, | |||
| InsertTextFormat: lsp.ITFSnippet, | |||
| Detail: candidate.Detail(), | |||
| // TODO: deprecated / tags? | |||
| // TODO: markdown handling | |||
There was a problem hiding this comment.
I assume it's safe to say this will always be plaintext until SDK actually allows providers to declare anything else, right?
There was a problem hiding this comment.
I think the markdown support should be RFCd properly, discussed with the SDK team and it may require further changes in the protocol as mentioned.
paultyng
force-pushed
the
documentation
branch
2 times, most recently
from
627963b to
4277725
Compare
Move descriptions to the documentation field, update to terraform-json 0.5.0 that has additional description support, etc.
|
@radeksimko I reworked the PR to surface the distinction more cleanly in line with what the LSP protocol should be in the future. Let me know your thoughts. |
👍
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
ghost
locked and limited conversation to collaborators
Move descriptions to the documentation field, update to terraform-json 0.5.0 that has additional description support, etc.
Using the
documentationinstead ofdetailgives it a better look in VS Code at least:(my misspelling aside)