#| highlighting alternative - #1092
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
juliasilge
left a comment
There was a problem hiding this comment.
I really like this approach, and I prefer it to #1084!
I have some inline comments so we are more consistent about what the special comments are, but I'd also like to ask that we make that background/underline/opacity treatment configurable. Can we add a configuration point similar to quarto.cells.background.color here so that folks can turn this off/on? I think boolean and default on is fine, but I would like to give folks a way to opt out of that visual treatment.
| const lines: number[] = []; | ||
| const lastLine = Math.min(blockRange.end.line, document.lineCount - 1); | ||
| for (let i = blockRange.start.line + 1; i <= lastLine; i++) { | ||
| if (!/^\s*# ?\|/.test(document.lineAt(i).text)) { |
There was a problem hiding this comment.
This regex does not match the same lines as Quarto's own option parser. That parser uses optionCommentPattern in cell/options.ts, which expands to /^#\s*\| ?/ for # languages and accepts any whitespace between # and |.
format.ts already imports optionCommentPattern from cell/options.ts so that its code path cannot drift from the option parser. Can we import it here too, instead of making a third copy of this pattern?
There was a problem hiding this comment.
hmmm. I see what you are saying but I am a bit confused: I played around with quarto render and the preview, and it seems that neither rendering nor previewing respect hash-pipe comments with spaces between i.e. # |. I was trying out:
# | echo: false
1 + 1
which still echoes the cell, v.s.
#| echo: false
1 + 1
which does not echo.
Am I missing something? Or is it the case that we should modify optionCommentPattern to be consistent with how preview and render work?
There was a problem hiding this comment.
Ok I looked into it a bit more and I now understand that knitr doesn't respect a space between (# |) but otherwise Quarto does. I guess I will not modify optionCommentPattern.
There was a problem hiding this comment.
We are now reusing optionCommentPattern in background and build-lang.
| while: (^|\\G)(?!\\s*([\`~]{3,})\\s*$) | ||
| contentName: ${contentName} | ||
| patterns: | ||
| - begin: ^(\\s*)(#\\|) |
There was a problem hiding this comment.
This rule matches only #|, but the engine also accepts interior spaces (optionCommentPattern expands to /^#\s*\| ?/), and the decoration in background.ts accepts # |. A line like # | echo: false gets the darker background but no YAML colors. Can we make this pattern match the same lines as optionCommentPattern?
There was a problem hiding this comment.
Also, this rule is stamped into every embedded language, but #| is only the option prefix for # comment languages. Cells in other languages use --| (sql, lua), //| (js, ojs, cpp), or %| (matlab) and get no YAML highlighting. The reverse also happens: a literal #| line in a css, json, or yaml block is mis-scoped as active YAML. The generator already emits one definition per language, so we can take the prefix per language (see kLangCommentChars in packages/core/src/jupyter/options.ts).
There was a problem hiding this comment.
We now use optionCommentPattern which addressed both of these
| contentName: ${contentName} | ||
| patterns: | ||
| - begin: ^(\\s*)(#\\|) | ||
| while: ^(\\s*)(#\\|) |
There was a problem hiding this comment.
This rule fires on any #| run in the cell body, not only the leading run, but Quarto only reads options from the leading run, and the new decoration in background.ts stops at the first non-option line. Is it possible for us to handle this the same way Quarto itself does?
```python
x = 1
#| eval: false
```
Maybe not with the declarative TextMate approach, in which case let's just document it.
There was a problem hiding this comment.
There's a way with TextMate! We now have a rule that captures option comments until there's a line that doesn't, and then all the rest of the lines get captured by another a separate regex.
|
|
||
| function clearEditorHighlightDecorations(editor: vscode.TextEditor) { | ||
| editor.setDecorations(highlightingConfig.backgroundDecoration(), []); | ||
| editor.setDecorations(cellOptionsBackgroundDecoration, []); |
There was a problem hiding this comment.
This clears the block decoration and both new cell option decorations, but not highlightingConfig.inlineBackgroundDecoration(). When the document's language mode changes, the inline backgrounds for `r ...` code stay behind on the non-Quarto document. That omission existed before this PR, but since we are adding clear calls here anyway, can we fix it? Just one line to do so, I believe.
| // to generalize to all languages (//| for js, --| for sql, /*| ... */ | ||
| // for c, etc.), derive the prefix from the block's language using | ||
| // kLangCommentChars/optionCommentPattern in packages/core/src/jupyter/options.ts | ||
| function hashPipeLines( |
There was a problem hiding this comment.
Building on the optionCommentPattern comment I added, the note at lines 256-259 defers //| and --| support, but the pieces already exist and we can bring them in a pretty straightforward way. The block token carries the language (languageNameFromBlock, exported from quarto-core), and kLangCommentChars in cell/options.ts maps each language to its comment prefix. It is private now, so it needs an export. With those two, this function works for js, sql, lua, and ojs cells too, and this third copy of "what is an option line" goes away.
There was a problem hiding this comment.
same as other comments, done by using optionCommentPattern!
|
@juliasilge thanks for the great comments. I have addressed them. I have also added a setting to turn on and off the background/underline/opacity. I have tested various .qmd files in both VSCode and Positron. I've also tested turning the setting on and off in both. I think the PR is in a good spot! |
juliasilge
left a comment
There was a problem hiding this comment.
This is really nice now! 🤩
I have two comments that it would be great to incorporate before merging, but no need for me to review again unless something comes up.
| "markdownDescription": "Millisecond delay between background color updates." | ||
| }, | ||
| "quarto.cells.options.background": { | ||
| "order": 23, |
There was a problem hiding this comment.
order: 23 is already taken by quarto.cells.hoverHelp.enabled (line 998). Every other quarto.cells.* setting has a unique order. With the tie, this one interleaves with the hover-help group in the Settings UI instead of the background settings.
The background group runs 19 to 22, so 23 is the right slot for this setting. Can we shift the other ones:
hoverHelp.enabled23 → 24signatureHelp.enabled24 → 25diagnostics.enabled25 → 26diagnostics.debounceDelay26 → 27useReticulate27 → 28
Co-authored-by: Julia Silge <julia.silge@gmail.com>
Fixes #409
This is an alternative approach to #1084.
This is a duplicate of #1088, which I accidentally merged to main and then reverted. Sorry for confusion.
This PR is a very simple change: it adds a yaml injection language for #| comments. This means that our syntax highlighting will delegate to the yaml language highlighter inside of #| comments.
The quarto.tmLanguage change is generated from the build-lang.js script.
IMO this is a more "correct" approach, if you will. We lose control over how the yaml looks, but we shouldn't really have that control anyway (let yaml be yaml)?
Pros
Cons
Testing note
We could test that the generated tmLanguage delegates
#|properly in some examples, in a non-integrated way, by usingvscode-textmateandvscode-onigurumaas dependencies in the tests, but that seems a little heavy handed so I didn't do that.