Support escaping snippet spaces#2223
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d03ee74d1f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return None | ||
|
|
||
| body = text[match_leading.start() :].rstrip() | ||
| body = rstrip_except_escaped_space(text[match_leading.start() :]) |
There was a problem hiding this comment.
Instead of my current special replacement of the rstrip, it might be better to just move the stripping logic to a better place.
There was a problem hiding this comment.
From the community backlog session, things overall look good. A couple points:
- I agree the escaping/stripping logic should be consolidated.
- Consider displaying errors in snippets with notifications and not just logging them as many users do not tend to leave the Talon log open. @AndreasArvidsson does this sound OK if we were to change the behavior of the error function?
Sounds good |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 076211c1f1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 083b84ce5d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| def insert_snippet(body: str): | ||
| # Avoid triggering VSCode's escaping mechanisms on accident | ||
| body = body.replace("\\", "\\\\") |
There was a problem hiding this comment.
When this VS Code override receives a snippet that already uses \$ to emit a literal dollar, doubling every backslash turns it into \\$, so VS Code parses an escaped backslash followed by a $exception variable instead of the literal $exception. This is already hit by the PHP catchStatement snippet (catch (\Throwable \$exception)) and by substitution text containing dollars, so inserting those snippets in VS Code produces the wrong text/placeholders; only escape literal backslashes without rewriting existing snippet escape sequences for $.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We should try to handle escaping logic ourselves so that snippets work across editors. I can fix it so that VSCode handles escaped stops properly.
There was a problem hiding this comment.
I reverted the change for now and will leave it to a followup.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed01ece73f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # treat double backslash as escaped backslash | ||
| # treat backslash space as space | ||
| return ( | ||
| body.replace("\\\\", UNICODE_ESCAPED_BACKSLASH_PLACEHOLDER) |
There was a problem hiding this comment.
When a snippet needs a literal backslash immediately before a placeholder, e.g. a custom VS Code body \\$1 under the newly documented escaped-backslash syntax, this replacement collapses it to \$1. That final body is then passed to VS Code's snippet engine in apps/vscode/vscode.py:446; VS Code's grammar treats backslash before $ as escaping the dollar (https://code.visualstudio.com/docs/editing/userdefinedsnippets#_grammar), so $1 is inserted as literal text instead of becoming a tab stop. This is separate from the reverted VS Code override: the fresh evidence is the parser now emitting \$ from escaped literal backslashes before placeholders.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I do not plan on addressing VSCode specific issues in this PR without maintainer feedback.
3 participants
No description provided.