That can render to markdown or plain text. Used for findHover requests.
Details
- Reviewers
malaperle sammccall kadircet - Commits
- rZORG75e760bef67f: [clangd] Introduce intermediate representation of formatted text
rZORG2b45d72c0850: [clangd] Introduce intermediate representation of formatted text
rG75e760bef67f: [clangd] Introduce intermediate representation of formatted text
rG2b45d72c0850: [clangd] Introduce intermediate representation of formatted text
rG5b0872fcfdfb: [clangd] Introduce intermediate representation of formatted text
rCTE360151: [clangd] Introduce intermediate representation of formatted text
rL360151: [clangd] Introduce intermediate representation of formatted text
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 31363 Build 31362: arc lint + arc unit
Event Timeline
This is a follow-up for a discussion from D55250.
Still missing test, wanted to get some input on the API first.
This looks pretty nice to me! Sorry for the wait.
Adding @kadircet as hover-guru-in-waiting.
clang-tools-extra/clangd/ClangdLSPServer.cpp | ||
---|---|---|
851 | (I'd like to see that in this patch if possible, it shouldn't be much work) | |
clang-tools-extra/clangd/ClangdServer.h | ||
184 | Not sure about switching from Hover to FormattedString as return type here: LSP hover struct contains a range field (what are the bounds of the hovered thing?) that we may want to populate that doesn't fit in FormattedString. (We'll eventually want ClangdServer to return some HoverInfo struct with structured information, as we want embedding clients to be able to render it other ways, and maybe use it to provide extension methods like YCM's GetType. But no need to touch that in this patch, we'll end up producing HoverInfo -> FormattedString -> LSP Hover, I guess) | |
clang-tools-extra/clangd/FormattedString.cpp | ||
42 | you may want to take the language name, and default it to either cpp or nothing. | |
64 | why do we add surrounding spaces if we don't do the same for text chunks? | |
clang-tools-extra/clangd/FormattedString.h | ||
2 | This will need tests as you note, which shouldn't be hard. | |
28 | I guess this will get an optional parameter to control the style (bold, italic etc)? | |
32 | Having various methods that take strings may struggle if we want a lot of composability (e.g. a bullet list whose bullet whose third word is a hyperlink/bold). (I'm not sure whether this is going to be a real problem, just thinking out loud here) One alternative extensibility would be to make "containers" methods taking a lambda that renders the body. FormattedString S; S << "std::"; S.bold([&} S << "swap" }; S.list(FormattedString::Numbered, [&] { S.item([&] { S << "sometimes you get the wrong overload"; }); S.item([&] { S << "watch out for "; S.link("https://en.cppreference.com/w/cpp/language/adl", [&] { S << "ADL"; }); }); }); S.codeBlock([&] S << "Here is an example"; }); Not sure if this is really better, I just have it on my mind because I ended up using it for the JSON.h streaming output. We can probably bolt it on later if necessary. | |
40 | I think we want renderForDebugging() or so which would print the chunks explicitly, a la CodeCompletionString. This is useful for actual debugging, and for testing methods that return FormattedString (as opposed to the FormattedString->markdown translation) | |
clang-tools-extra/clangd/XRefs.cpp | ||
536 | should this be an inline code block? | |
clang-tools-extra/unittests/clangd/XRefsTests.cpp | ||
1157 ↗ | (On Diff #187936) | When this lands for real, I think we should assert on the renderForDebugging() output or similar. |
clang-tools-extra/clangd/ClangdServer.h | ||
---|---|---|
184 | I agree with Sam on the layering. I was also working in a patch that was changing ClangdServers output from Hover to HoverInfo(a structured version of Hover). |
- Rebase
- Parse client capabilities, send markdown on hover if client supports it
- Use inline block for the scope
- Add a language for code blocks
- Add a FIXME for hover tests
- Fix escaping, add tests
- Update XRefs tests
clang-tools-extra/clangd/FormattedString.cpp | ||
---|---|---|
42 | Done. Defaulting to 'cpp'. | |
64 | a leftover from my first prototype. | |
clang-tools-extra/clangd/FormattedString.h | ||
32 | Agree that the current approach won't generalize to more complicated cases without some design work. I also think we should bolt on this until we hit more use-cases with more extensive needs for formatting markdown. | |
40 | That would be a useful addition. I've added a FIXME for now and testing plaintext in XRefs. That would need updates to all hover tests and that's a big enough to go into a separate change, from my perspective. |
clang-tools-extra/clangd/ClangdServer.h | ||
---|---|---|
184 | ClangdServer does not know whether to render the FormattedString into markdown or plaintext and I believe it should stay that way. |
- Return 'HoverInfo' instead of FormattedString
- Reformat
clang-tools-extra/clangd/ClangdServer.h | ||
---|---|---|
184 | Returning HoverInfo now, with a FormattedString and a range. |
clang-tools-extra/clangd/FormattedString.h | ||
---|---|---|
28 | Either that or a separate function to add bold/italic, etc. This really tries to provide a vocabulary type to carry a string with formatted blocks around, not support full markdown from the start. |
This is ready for another round of review. As mentioned in the comments, I think some of the comments are better addressed by a separate change, because they would touch lots of lines in the test.
Let me know if I missed anything else.
As discussed, it probably makes sense to split into two patches, adding this abstraction and using it.
(All together is also fine but should have the tests for the new behavior, which probably means renderForTests() too)
Generally looks good. I think we'll run into the limitations as soon as we want to add lists, but that's OK.
clang-tools-extra/clangd/FormattedString.cpp | ||
---|---|---|
72 | backticks = "```" while (Input.contains(backticks)) backticks.push_back("`") I don't think we care about DOS here? | |
91 | comment for this merge? | |
108 | I'm not sure we're realistically going to be able to enforce this very well, we're going to use this for types. Maybe translate into ' ' instead? (or at least in production mode) | |
141 | (we could also use the version in CharInfo) |
- Use isWhitespace from CharInfo.
- Add a comment for merging text blocks together.
- Remove assertion that inline code block does not contain newlines.
- Simplify how the max number of backticks is computed.
clang-tools-extra/clangd/FormattedString.cpp | ||
---|---|---|
72 | I'd like to keep this linear anyway. | |
108 | Actually, the CommonMark specification allows newline in the inline code blocks. | |
141 | Thanks! I knew there should be a helper like this somewhere in LLVM... |
clang-tools-extra/clangd/FormattedString.h | ||
---|---|---|
35 | newlines are allowed now? |
(I'd like to see that in this patch if possible, it shouldn't be much work)