-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CLIENT-SPECIFICATION: fix typo, add a way to escape the placeholder syntax #10730
Conversation
### Examples | ||
|
||
- `ping {{example.com}}` MUST be rendered as "ping example.com" | ||
- `docker inspect --format '\{\{range.NetworkSettings.Networks\}\}\{\{.IPAddress\}\}\{\{end\}\}' {{container}}` MUST be rendered as "docker inspect --format '{{range.NetworkSettings.Networks}}{{.IPAddress}}{{end}}' container" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would personally lighten the syntax to only one backslash:
docker inspect --format '\{{range.NetworkSettings.Networks\}}\{{.IPAddress\}}\{{end\}}' {{container}}
.
This is maybe easier to parse than what I had proposed, but still not too heavy for reading source pages (even though this isn't done often). The drawback could if we needed to render something like \{{ <...> }}
But since I've seen only 3 occurrences of this problem, maybe this change is not needed. 🤷🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I favor a backslash for each curly brace. I think it's much more intuitive to treat things per character, rather than by our special definition attributed to combinations of characters.
In Markdown, \{
already means a literal {
. So we should run with it. This will also be more backward compatible. When we don't mean the placeholder syntax, {{
is never found, so even if the client doesn't know to ignore it, it'll inadvertently ignore it anyway.
This will be simpler than giving {{
conditional meaning based on an escape sequence multiple characters away.
If a client finds \{{range.NetworkSettings.Networks\}}
, and processes placeholders before delegating to the CommonMark library, it's probably going to mess up unless it knows to check for a backslash. If a client finds \{\{range.NetworkSettings.Networks\}\}
without any special handling, it knows it's not a placeholder, and the CommonMark library will just do its magic.
For nested curly braces inside the placeholder syntax:
Syntax | Arg (curly braces removed) | Rendered |
---|---|---|
{{uwu}} |
uwu |
uwu |
{{\{uwu\}}} |
\{uwu\} |
{uwu} |
{{\{\{uwu\}\}}} |
\{\{uwu\}\} |
{{uwu}} |
The main difference, here, is that having a single escape sequence affect multiple characters would require special handling from clients, while doing one escape sequence per character is already supported/expected by clients and CommonMark parsers. The benefits assume the client is using a CommonMark library to parse the page.
I found a big issue with this. Currently, everything inside an example is treated literally, so rendering backslashes in any position is not a problem. But if we want to allow every For example, how would this be rendered (Windows page for One solution is to only allow double brackets to be escaped ( this is the official C client, trying to render No page requires a single pair of curly braces inside a placeholder as of now, but it would be great to cover everything. The PR as it is now would require escaping every brace that is supposed to be literal, but then also every backslash must be escaped, resulting in the need to rewrite all Windows pages like this:
and to complicate every client's page parser to allow for this, which is not a very good solution at all. |
I think |
Is this one ready to be merged? |
Nope, Client specification PRs (especially new requirement ones) are usually kept open the longest (in some cases months) to allow maintainers and community members (client authors, etc) to comment and propose additions/removals to the PR. We can set a tentative merge date for this next month probably on the 17th. |
Co-authored-by: Lena <126529524+acuteenvy@users.noreply.github.com>
@acuteenvy Let me know if the applied changes are fine. I am planning to merge this tomorrow. Will work on some other related PRs for the Client Specification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still a problem with Windows paths. If \{\{
means escaping and \{{
should be a literal backslash, we have to make it unambiguous.
Co-authored-by: Lena <126529524+acuteenvy@users.noreply.github.com>
Examples proposed by @mrnossiom in #10148 (comment).
Closes #10125.