Eliminate the snippet source #782
Conversation
| "lib": { | ||
| "prefix": "lib", | ||
| "body": "library(${1:package})", | ||
| "description": "Attach an R package" | ||
| }, | ||
| "src": { | ||
| "prefix": "src", | ||
| "body": "source(\"${1:file.R}\")", | ||
| "description": "Source an R file" | ||
| }, |
There was a problem hiding this comment.
The lib and src snippets just go away.
library() completes normally and we already have ✨ magic ✨ package name completions inside library(). source() completes normally and filepath completions will kick in inside source().
| "ret": { | ||
| "prefix": "ret", | ||
| "body": "return(${1:code})", | ||
| "description": "Return a value from a function" | ||
| }, |
There was a problem hiding this comment.
return() is now showing up as a "normal" completion, as of #768, coming from the search path source.
| "mat": { | ||
| "prefix": "mat", | ||
| "body": "matrix(${1:data}, nrow = ${2:rows}, ncol = ${3:cols})", | ||
| "description": "Define a matrix" | ||
| }, | ||
| "sg": { | ||
| "prefix": "sg", | ||
| "body": [ | ||
| "setGeneric(\"${1:generic}\", function(${2:x, ...}) {", | ||
| "\tstandardGeneric(\"${1:generic}\")", | ||
| "})" | ||
| ], | ||
| "description": "Define a generic" | ||
| }, | ||
| "sm": { | ||
| "prefix": "sm", | ||
| "body": [ | ||
| "setMethod(\"${1:generic}\", ${2:class}, function(${2:x, ...}) {", | ||
| "\t${0}", | ||
| "})" | ||
| ], | ||
| "description": "Define a method for a generic function" | ||
| }, | ||
| "sc": { | ||
| "prefix": "sc", | ||
| "body": "setClass(\"${1:Class}\", slots = c(${2:name = \"type\"}))", | ||
| "description": "Define a class definition" | ||
| }, |
There was a problem hiding this comment.
mat for matrix(), sg for setGeneric(), sm for setMethod(), sc for setClass() go away.
| "if": { | ||
| "prefix": "if", | ||
| "body": [ | ||
| "if (${1:condition}) {", | ||
| "\t${0}", | ||
| "}" | ||
| ], | ||
| "description": "Conditional expression" | ||
| }, | ||
| "el": { | ||
| "prefix": "el", | ||
| "body": [ | ||
| "else {", | ||
| "\t${0}", | ||
| "}" | ||
| ], | ||
| "description": "Conditional expression" |
There was a problem hiding this comment.
if and else are now covered in the keyword source. We should discuss whether we want snippet-y treatment (current treatment), just plain words, or (conceivably) both.
| "ei": { | ||
| "prefix": "ei", | ||
| "body": [ | ||
| "else if (${1:condition}) {", | ||
| "\t${0}", | ||
| "}" | ||
| ], | ||
| "description": "Conditional expression" | ||
| }, |
There was a problem hiding this comment.
The else if snippet just goes away.
| "fun": { | ||
| "prefix": "fun", | ||
| "body": [ | ||
| "${1:name} <- function(${2:variables}) {", | ||
| "\t${0}", | ||
| "}" | ||
| ], | ||
| "description": "Function skeleton" | ||
| }, |
There was a problem hiding this comment.
The snippet-y treatment of function has moved to the keyword source, which is also now providing a bare completion of function as well, thanks to #768 + another move in this PR. This is what it looks like when we cover a reserved word with 2 different kinds of completion item. This merits discussion.
There was a problem hiding this comment.
I think this is fine. I like the idea of providing the non-snippet keyword even if we also provide a snippet, especially for people who generally don't love snippets
| "for": { | ||
| "prefix": "for", | ||
| "body": [ | ||
| "for (${1:variable} in ${2:vector}) {", | ||
| "\t${0}", | ||
| "}" | ||
| ], | ||
| "description": "Define a loop" | ||
| }, | ||
| "while": { | ||
| "prefix": "while", | ||
| "body": [ | ||
| "while (${1:condition}) {", | ||
| "\t${0}", | ||
| "}" | ||
| ], | ||
| "description": "Define a loop" | ||
| }, |
There was a problem hiding this comment.
for and while have moved to the keyword source. Currently as snippets only.
There was a problem hiding this comment.
Now handled with both a bare completion and a snippet.
| "switch": { | ||
| "prefix": "switch", | ||
| "body": [ | ||
| "switch (${1:object},", | ||
| "\t${2:case} = ${3:action}", | ||
| ")" | ||
| ], | ||
| "description": "Define a switch statement" | ||
| }, | ||
| "apply": { | ||
| "prefix": "apply", | ||
| "body": "apply(${1:array}, ${2:margin}, ${3:...})", | ||
| "description": "Use the apply family" | ||
| }, | ||
| "lapply": { | ||
| "prefix": "lapply", | ||
| "body": "lapply(${1:list}, ${2:function})", | ||
| "description": "Use the apply family" | ||
| }, | ||
| "sapply": { | ||
| "prefix": "sapply", | ||
| "body": "sapply(${1:list}, ${2:function})", | ||
| "description": "Use the apply family" | ||
| }, | ||
| "mapply": { | ||
| "prefix": "mapply", | ||
| "body": "mapply(${1:function}, ${2:...})", | ||
| "description": "Use the apply family" | ||
| }, | ||
| "tapply": { | ||
| "prefix": "tapply", | ||
| "body": "tapply(${1:vector}, ${2:index}, ${3:function})", | ||
| "description": "Use the apply family" | ||
| }, | ||
| "vapply": { | ||
| "prefix": "vapply", | ||
| "body": "vapply(${1:list}, ${2:function}, FUN.VALUE = ${3:type}, ${4:...})", | ||
| "description": "Use the apply family" | ||
| }, | ||
| "rapply": { | ||
| "prefix": "rapply", | ||
| "body": "rapply(${1:list}, ${2:function})", | ||
| "description": "Use the apply family" | ||
| }, | ||
| "ts": { | ||
| "prefix": "ts", | ||
| "body": "`r paste(\"#\", date(), \"------------------------------\\n\")`", | ||
| "description": "Insert a datetime" | ||
| }, |
There was a problem hiding this comment.
All of these snippets go away: switch, the apply family, ts for an executable snippet (which we don't support, in any case).
| "shinyapp": { | ||
| "prefix": "shinyapp", | ||
| "body": [ | ||
| "library(shiny)", | ||
| "", | ||
| "ui <- fluidPage(", | ||
| " ${0}", | ||
| ")", | ||
| "", | ||
| "server <- function(input, output, session) {", | ||
| " ", | ||
| "}", | ||
| "", | ||
| "shinyApp(ui, server)" | ||
| ], | ||
| "description": "Define a Shiny app" | ||
| }, | ||
| "shinymod": { | ||
| "prefix": "shinymod", | ||
| "body": [ | ||
| "${1:name}_UI <- function(id) {", | ||
| " ns <- NS(id)", | ||
| " tagList(", | ||
| "\t${0}", | ||
| " )", | ||
| "}", | ||
| "", | ||
| "${1:name} <- function(input, output, session) {", | ||
| " ", | ||
| "}" | ||
| ], | ||
| "description": "Define a Shiny module" | ||
| } |
There was a problem hiding this comment.
Perhaps the shiny extension should provide these?
There was a problem hiding this comment.
It is actually already in the business of providing snippets, so that's nice:
| // The only case where `keyword != label` is `fun` for `function`. | ||
| // But in the name of preserving original behaviour, this is my opening | ||
| // move. | ||
| const KEYWORD_SNIPPETS: &[(&str, &str, &str, &str)] = &[ |
There was a problem hiding this comment.
Transplanting existing completion treatment of for, while, if, else, and fun (for a function snippet) from snippet source to the keyword source.
| "Define a loop", | ||
| ), | ||
| ( | ||
| "else", | ||
| "else", |
There was a problem hiding this comment.
Using else as the label for the else snippet, instead of el, means that the el() function in the methods package now appears in the completion list.
| "next", | ||
| "break", | ||
| "repeat", | ||
| "function", |
There was a problem hiding this comment.
Transplanted the bare function completion from search path source to here (keyword source).
| const R_CONTROL_FLOW_KEYWORDS: &[&str] = &[ | ||
| "if", "else", "for", "in", "while", "repeat", "break", "next", | ||
| const KEYWORD_SOURCE: &[&str] = &[ | ||
| "if", "else", "for", "in", "while", "repeat", "break", "next", "function", |
There was a problem hiding this comment.
The other side of transplanting the bare function completion to the keyword source.
| item.detail = Some("[keyword]".to_string()); | ||
| item.kind = Some(CompletionItemKind::KEYWORD); | ||
| item.label_details = Some(CompletionItemLabelDetails { | ||
| detail: None, | ||
| description: Some("[keyword]".to_string()), | ||
| }); |
There was a problem hiding this comment.
The keyword completion items were actually using the fields of CompletionItem differently than the other sources, leading to undesirable behaviour in the UI. This [keyword] wouldn't show up by default, the way that, say, {methods} would. Instead it would only show up if that item was queued for selection. Here's how things look with this change:
| }; | ||
|
|
||
| item.documentation = Some(Documentation::MarkupContent(markup)); | ||
| item.kind = Some(CompletionItemKind::SNIPPET); |
There was a problem hiding this comment.
The use of CompletionItemKind::SNIPPET is mildly controversial, but I also think it's the best move. The alternative would be CompletionItemKind::KEYWORD. But if we're inserting a snippet (which we currently are), I think we need to use the snippet icon. Arguably we should also provide bare completions for, e.g., for, which would be CompletionItemKind::KEYWORD. But then we will have to expand the key of our completion map to go beyond label. Perhaps label and kind?
function currently appears as a snippet, via fun, and as a bare keyword as function (which is why they don't collide). I think it would be weird if these two entries had the same icon.
There was a problem hiding this comment.
It turns out ☝️ is exactly what VS Code does for the handful of languages for which it provides built-in snippets. Check out this this dual treatment of for for javascript:
https://code.visualstudio.com/docs/editing/userdefinedsnippets#_builtin-snippets
There was a problem hiding this comment.
Yea I honestly really approve of the fact that for is represented twice:
- Once as a keyword, because that's what it is. It's part of the language.
- Once as a special snippet, which the user should be able to turn off if they want to!
jennybc
force-pushed
the
no-built-in-snippets
branch
from
75b83e6 to
3a8f91c
Compare
There was a problem hiding this comment.
Looks great, my main comment is about wanting to be consistent in how we treat snippet keywords vs bare keywords.
i.e. I think repeat, for, and while should all be treated the same, and currently aren't.
I feel like repeat should move to KEYWORD_SNIPPETS for consistency, and possibly all KEYWORD_SNIPPETS should also have a corresponding bare keyword.
| "fun": { | ||
| "prefix": "fun", | ||
| "body": [ | ||
| "${1:name} <- function(${2:variables}) {", | ||
| "\t${0}", | ||
| "}" | ||
| ], | ||
| "description": "Function skeleton" | ||
| }, |
There was a problem hiding this comment.
I think this is fine. I like the idea of providing the non-snippet keyword even if we also provide a snippet, especially for people who generally don't love snippets
| "for", | ||
| "for", | ||
| "for (${1:variable} in ${2:vector}) {\n\t${0}\n}", | ||
| "Define a loop", |
There was a problem hiding this comment.
You could probably define a const constructible type that is self documenting. Like you'd replace this with
KeywordSnippet {
keyword: "for",
label: "for",
snippet: "for (${1:variable} in ${2:vector}) {\n\t${0}\n}",
label_details_description: "Define a loop"
}
Which would be nicely self documenting and cleanly unpack in the loop like
for KeywordSnippet { keyword, label, snippet, label_details_description } in KEYWORD_SNIPPETS {
| }; | ||
|
|
||
| item.documentation = Some(Documentation::MarkupContent(markup)); | ||
| item.kind = Some(CompletionItemKind::SNIPPET); |
There was a problem hiding this comment.
Yea I honestly really approve of the fact that for is represented twice:
- Once as a keyword, because that's what it is. It's part of the language.
- Once as a special snippet, which the user should be able to turn off if they want to!
|
@DavisVaughan Yeah I have convinced myself that it makes sense to represent something like |
|
Great, let's try representing all |
Part of #7234 In posit-dev/ark#782, I remove the so-called snippet source of completions in ark. Previously, the completions coming from the snippet source were a mix of snippets related to R reserved words (such as `if`, `while`, or `for`) and a collection of other snippets relating to various functions in base R (such as `matrix()` or `lapply()`). This was an eclectic mix of snippets inherited from RStudio. After [#782](posit-dev/ark#782), completion handling for reserved words lives with ark's keyword source. The usual snippets still appear (or do not), in the same contexts, just via a different internal mechanism. Functions like `matrix()` or `lapply()`, however, no longer have special handling and receive the same completion treatment as any other function on the search path. If users want such snippets, we're going to follow the existing VS Code vibe, which is to recommend that the user configure custom snippets, via the command palette and *Snippets: Configure Snippets*: <img width="218" alt="Screenshot 2025-04-25 at 3 34 23 PM" src="https://github.com/user-attachments/assets/0b167aec-5ae8-4798-843f-4691027842fb" /> A newly created snippet file using this interface is pre-populated with a big comment that explains what sort of content to place in the file. Snippet files comes in 2 main flavors: * "global": contains snippets that are not limited to one language. Individual snippets in such a global file can have a `scope` which specifies one or more languages. Created via `createSnippetFile()`. <img width="607" alt="Screenshot 2025-04-25 at 3 38 00 PM" src="https://github.com/user-attachments/assets/b2c039a3-65dc-451a-9b3a-fd162d08e537" /> * language-specific: contains snippets for one specific language. Created via `createLanguageSnippetFile()`. <img width="301" alt="Screenshot 2025-04-25 at 3 45 05 PM" src="https://github.com/user-attachments/assets/facb30ca-4e20-4c84-8741-68fe30e235f7" /> This PR modifies the placeholder content for both functions. The main idea is to give examples for R and Python, instead of javascript/typescript. This PR is similar in spirit to other changes we've made to make default or example content more data science / R / Python oriented. The mechanics are pretty bone-headed, but that is motivated by our overlay strategy and making things as easy as possible for upstream merges. *I plan to complement this PR with a companion PR to the website aimed at helping snippet users coming from RStudio.*
Soon, several control flow elements will have both a bare and a snippet representation and we'll use the same `label` (but obviously differentiate elsewhere)
| fn new(item: &CompletionItem) -> Self { | ||
| Self { | ||
| label: item.label.clone(), | ||
| kind_str: item |
There was a problem hiding this comment.
We can't just use the completion item kind here, without dealing with a hash implementation for that enum. So I decided to just use a string representation, which is convenient for logging anyway.
Deal with reserved words in the order in which they appear in their help topic and be consistent everywhere
jennybc
force-pushed
the
no-built-in-snippets
branch
from
1ba2b20 to
0104a3b
Compare
Closes #779
Closes #780
Addresses posit-dev/positron#3108
Relates to posit-dev/positron#7234. Morally, it doesn't quite close that issue, because we might want to do some things on the Positron side, such as (1) add R and/or Python examples to the placeholder content of newly created snippet files (now in posit-dev/positron#7401) and (2) add documentation about R snippets in Positron vis-a-vis how it works in RStudio (now in posit-dev/positron-website#67).