Add hover #107
Add hover #107
Conversation
|
Makdown content has been enhanced ✨ |
src/scry/hover_provider.cr
Outdated
| context.each do |key, value| | ||
| contents << "| #{key} | #{value} |" | ||
| escaped_value = "`#{value.gsub("|", "∣")}`" |
There was a problem hiding this comment.
is that a special unicode pipe? if yes, can you add a comment saying that (and maybe the unicode code (in hexa) too)
src/scry/hover_provider.cr
Outdated
| end | ||
|
|
||
| private def vertical_align(contexts) | ||
| contents = [] of String |
src/scry/hover_provider.cr
Outdated
| private def vertical_align(contexts) | ||
| contents = [] of String | ||
| contexts.each_with_index do |context, i| | ||
| contents << "**Context #{i + 1}**\n" |
There was a problem hiding this comment.
If there is only one context we should hide the Context 1/2/...
src/scry/hover_provider.cr
Outdated
| titles_lines = ["| :--- |"] of String | ||
| contents_hash = {} of String => Array(String) | ||
| contexts.each_with_index do |context, i| | ||
| titles << " Type #{i + 1} |" |
|
Changed to vertical layout and group per context 🎉 |
src/scry/hover_provider.cr
Outdated
| contexts.each_with_index do |context, i| | ||
| contents << "**Context #{i + 1}**\n" if contexts.size > 1 | ||
| contents << "```crystal\n" | ||
| max_size = context.keys.max_by(&.size).size |
There was a problem hiding this comment.
can be calculated outside the each_with_index loop
src/scry/hover_provider.cr
Outdated
| contents << "```crystal\n" | ||
| max_size = context.keys.max_by(&.size).size | ||
| context.each do |key, value| | ||
| key_aligned = "#{key}".ljust(max_size) |
There was a problem hiding this comment.
key is already a String, no?
So: key.ljust(max_size)
src/scry/hover_provider.cr
Outdated
| contents << "**Context #{i + 1}**\n" if contexts.size > 1 | ||
| contents << "```crystal\n" | ||
| max_size = context.keys.max_by(&.size).size | ||
| context.each do |key, value| |
There was a problem hiding this comment.
key => var_name
value => var_type (or juste type ?)
wdyt?
src/scry/hover_provider.cr
Outdated
| # | arg1 | `String` | `String` | | ||
| # | arg2 | `Int32 ∣ Nil` | `Bool` | | ||
| private def horizontal_align(contexts) | ||
| contents = [] of String |
There was a problem hiding this comment.
You could build this one with an IO::Memory too, by generating the output line by line (first the header, then the lines, then the types)
I think it would lead to cleaner/readable code
|
#95 is already merged 🎉 , waiting for more reviews... |
src/scry/protocol/hover.cr
Outdated
| JSON.mapping({ | ||
| contents: MarkupContent, | ||
| range: Range?, | ||
| }, true) |
There was a problem hiding this comment.
Use named arg static: true here, 'cause by just reading true I have no idea what it refers to for a json mapping, I think it's better with the named arg
There was a problem hiding this comment.
Also, why only this mapping have this static check?
There was a problem hiding this comment.
static: true is not needed here anymore, I'm already fixing that on lsp shard
Lets remove it for now 👍
|
@faustinoaq More of an overall comment, I noticed a lot of the code between |
Fixes #103 🎉
Depends on #95Merged! 🎉This uses
crystal tool contextto get context information