feat: add a test for placeholder escaping #9
Conversation
| @test "REQUIRED: supports escaping the placeholder syntax" { | ||
| run $PATH_TO_TLDR_CLIENT docker inspect | ||
| [[ $status -eq 0 ]] | ||
| [[ "$output" = *"{{range.NetworkSettings.Networks}}{{.IPAddress}}{{end}}"* ]] |
There was a problem hiding this comment.
| [[ "$output" = *"{{range.NetworkSettings.Networks}}{{.IPAddress}}{{end}}"* ]] | |
| [[ "$output" = *"'{{range.NetworkSettings.Networks}}{{.IPAddress}}{{end}}' container`"* ]] |
There was a problem hiding this comment.
I don't think that suggestion adds much value, meanwhile it does add potential complexity with handling different styles, colors, or ANSI codes between arguments in different clients.
I think we should stick to the original proposal.
Reference: #9 (comment)
There was a problem hiding this comment.
Maybe split this into the past version and this suggestion, making the latter optional, with NO_COLOR set to true? I do think that the backslashes can confuse clients.
Well, the other test already tests the placeholder between the escaped curly brackets, right? So what's the point here?
There was a problem hiding this comment.
Which test are you referring to there?
I don't think we're testing escaped curly braces yet. It was a relatively recent addition to the spec compared to when I wrote these tests.
There was a problem hiding this comment.
Sorry, I was talking about the other line of this test
There was a problem hiding this comment.
These two tests are different.
- One tests that escaped curly braces are rendered correctly.
\{\{arg\}\}→{{arg}} - One ensures that double backslash (normally interpreted as escaped backslashes) are rendered correctly. It does not test escaped curly braces.
\\arg→\\arg
There was a problem hiding this comment.
Sure, I'm not saying they are equivalent. But the double backslash test, want it or not, also tests the rendering of the placeholders, so it should theoretically suffer from the same problems you mentioned
There was a problem hiding this comment.
I don't understand what you're trying to say. These are both handling entirely different scenarios. It's possible to craft input that could pass or fail both, but this is about how escapes are handled, which any clients could handle one escape correctly but not the other.
There was a problem hiding this comment.
See Microsoft documentation:
mount -o anon \ServerIP\ShareName Z:
The original Markdown is:
mount \{{computer_name}}{{share_name}} {{Z:}}
If I'm getting it right, it should be rendered as:
mount \{{computer_name}}{{share_name}} {{Z:}}
Yeah, they are different, because the mount page doesn't test escaped placeholders, but it does test text between the placeholders, so it could theorically suffer from the problems you mentioned such as ANSI codes.
I hope you understand me now 😅
There was a problem hiding this comment.
Ahh, I'm following now! Yeah, the second test is actually written wrong then in that case.
Don't include curly braces in tests, clients are free to remove/replace them.
That really should say placeholder syntax, escaped curly braces are fine. Clients are free to render it as \\{{computer_name}}\{{share_name}} or \\computer_name\share_name, usually if a client does the latter they use colors/styles to signify the args.
It should only check what it wants to test, so the \\ part, or we could check for multiple tokens like \\ and computer_name, but the assertions should not include the {{.
|
Thanks for pinging me, I forgot to watch the repo after transferring it. 😅 |
| [[ "$output" = *"{{range.NetworkSettings.Networks}}{{.IPAddress}}{{end}}"* ]] | ||
| run $PATH_TO_TLDR_CLIENT --platform windows mount | ||
| [[ $status -eq 0 ]] | ||
| [[ "$output" = *"\\computer_name\share_name"* ]] |
There was a problem hiding this comment.
This test and the associated specifications are odd to me. If we are using CommonMark, then \\ should become a single backslash.
The page should have \\\\{computer_name}\\{share_name}, which would render out to \\{computer_name}\{share_name}, right?
With the specifications as they are written, I don't think the specifications make sense. This does look to me like it tests what the specifications intended, though.
| @test "REQUIRED: supports escaping the placeholder syntax" { | ||
| run $PATH_TO_TLDR_CLIENT docker inspect | ||
| [[ $status -eq 0 ]] | ||
| [[ "$output" = *"{{range.NetworkSettings.Networks}}{{.IPAddress}}{{end}}"* ]] |
There was a problem hiding this comment.
I don't think that suggestion adds much value, meanwhile it does add potential complexity with handling different styles, colors, or ANSI codes between arguments in different clients.
I think we should stick to the original proposal.
Reference: #9 (comment)
The backslashes are inside a code block, so they would be treated literally even by a CommonMark parser.
The current client specification doesn't strictly follow CommonMark. Single braces can't be escaped -
The only other way is to release a specification which states that every brace can be escaped, and all backslashes must be. The problem with that is, most clients will display double backslashes everywhere for a while.
https://github.com/tldr-pages/tldr/blob/main/CLIENT-SPECIFICATION.md#page-structure
If a client removes backlashes and doesn't remove braces that mark the placeholder, the user won't know which braces are part of the command. |
Ahh, you're right. I forgot they were in inline code. It seems I made the same mistake during the initial discussion for that revision to spec, too. ^-^'
Understood, I misinterpreted the spec, thank you for clarifying this. I would still expect us to use the approach proposed (checking "token" at a time) to avoid issues with formatting, ANSI codes, etc. However, indeed we should disregard my comments regarding curly braces being optional. In a separate commit, I will also remove the comment I've left in the test file about this. In that case, this all looks good to me. Thanks for adding the test, and being patient with me and my misguided review. 😅 |
No description provided.