Skip to content
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.

Sign up for GitHub

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

feat: add a test for placeholder escaping #9

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add a test for placeholder escaping #9

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions tldr.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,17 @@ setup() {
[[ "$output" = *"https://manned.org/7z"* ]]
}

# Source: https://github.com/tldr-pages/tldr/blob/main/CLIENT-SPECIFICATION.md#page-structure
# bats test_tags=required
@test "REQUIRED: supports escaping the placeholder syntax" {
run $PATH_TO_TLDR_CLIENT docker inspect
[[ $status -eq 0 ]]
[[ "$output" = *"{{range.NetworkSettings.Networks}}{{.IPAddress}}{{end}}"* ]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[[ "$output" = *"{{range.NetworkSettings.Networks}}{{.IPAddress}}{{end}}"* ]]
[[ "$output" = *"'{{range.NetworkSettings.Networks}}{{.IPAddress}}{{end}}' container`"* ]]

Copy link
Member

@SethFalco SethFalco Mar 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@vitorhcl vitorhcl Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was talking about the other line of this test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two tests are different.

  1. One tests that escaped curly braces are rendered correctly. \{\{arg\}\} → {{arg}}
  2. One ensures that double backslash (normally interpreted as escaped backslashes) are rendered correctly. It does not test escaped curly braces. \\arg → \\arg

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Member

@SethFalco SethFalco Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {{.

run $PATH_TO_TLDR_CLIENT --platform windows mount
[[ $status -eq 0 ]]
[[ "$output" = *"\\computer_name\share_name"* ]]
Copy link
Member

@SethFalco SethFalco Mar 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

# Source: https://github.com/tldr-pages/tldr/blob/main/CLIENT-SPECIFICATION.md#arguments
# bats test_tags=optional
@test "OPTIONAL: supports --list argument to list all pages" {
Expand Down