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.

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

fakedata: add page #5675

Merged
merged 12 commits into from
Apr 10, 2021
Merged

fakedata: add page #5675

merged 12 commits into from
Apr 10, 2021

Conversation

nicokosi
Copy link
Collaborator

@nicokosi nicokosi commented Apr 3, 2021

  • The page (if new), does not already exist in the repo.
  • The page is in the correct platform directory (common/, linux/, etc.)
  • The page has 8 or fewer examples.
  • The PR title conforms to the recommended templates.
  • The page follows the content guidelines.
  • The page description includes a link to documentation or a homepage (if applicable).

@tldr-bot
Copy link

tldr-bot commented Apr 3, 2021

The build for this PR failed with the following error(s):

pages/common/fakedata.md:24 MD047/single-trailing-newline Files should end with a single newline character
pages/common/fakedata.md:1: TLDR009 Page should contain a newline at end of file

Please fix the error(s) and push again.

@bl-ue bl-ue added the new command Issues requesting creation of a new page. label Apr 3, 2021
pages/common/fakedata.md Outdated Show resolved Hide resolved
pages/common/fakedata.md Outdated Show resolved Hide resolved
pages/common/fakedata.md Outdated Show resolved Hide resolved
Co-authored-by: bl-ue <54780737+bl-ue@users.noreply.github.com>
@nicokosi
Copy link
Collaborator Author

nicokosi commented Apr 3, 2021

The build for this PR failed with the following error(s):

pages/common/fakedata.md:24 MD047/single-trailing-newline Files should end with a single newline character
pages/common/fakedata.md:1: TLDR009 Page should contain a newline at end of file

Please fix the error(s) and push again.

Error seems outdated. 😇

pages/common/fakedata.md Outdated Show resolved Hide resolved
pages/common/fakedata.md Outdated Show resolved Hide resolved
pages/common/fakedata.md Outdated Show resolved Hide resolved
pages/common/fakedata.md Outdated Show resolved Hide resolved
Co-authored-by: bl-ue <54780737+bl-ue@users.noreply.github.com>
pages/common/fakedata.md Outdated Show resolved Hide resolved
@nicokosi
Copy link
Collaborator Author

nicokosi commented Apr 3, 2021

Thank you @bl-ue for the typos and the reword suggestions. 😎

pages/common/fakedata.md Outdated Show resolved Hide resolved
Co-authored-by: bl-ue <54780737+bl-ue@users.noreply.github.com>
Copy link
Collaborator

@marchersimon marchersimon left a comment

Choose a reason for hiding this comment

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

LGTM, however it really confused me that data is the plural of datum and I fear it may confuse others too.

@bl-ue
Copy link
Contributor

bl-ue commented Apr 3, 2021

Let's see what others have to say about that then. @sbrl

@bl-ue bl-ue requested a review from sbrl April 3, 2021 17:58

- Generate data using a custom output template (capitalize the first letter of generators):

`echo "value: {{Generator}}" | fakedata`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
`echo "value: {{Generator}}" | fakedata`
`echo "value: {{generator}}" | fakedata`

Copy link
Collaborator Author

@nicokosi nicokosi Apr 3, 2021

Choose a reason for hiding this comment

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

I used "Pascal case" on purpose because:

If the generator name is a single word, then it's available as a function with the same name capitalized (example: int becomes Int). If the generator name is composed by multiple words joined by dots, then the function name is again capitalized by the first letter of the word and joined together (example: product.name becomes Product.Name).

See https://github.com/lucapette/fakedata#generators-1

What do you think of this "intentional violation"? 😇

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 all the custom format is the custom argument option in this example, so the valid highlight is the following:

Suggested change
`echo "value: {{Generator}}" | fakedata`
`echo "{{value: Generator}}" | fakedata`

Copy link
Contributor

@bl-ue bl-ue Apr 3, 2021

Choose a reason for hiding this comment

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

I prefer the original capitalized Generator to generator. It's required anyway. @navarroaxel see the example description itself; it mentions this oddity.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nicokosi maybe it would be better to change the parenthetic message to the first letter of generator names must be capitalized, that sounds a bit clearer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nicokosi maybe it would be better to change the parenthetic message to the first letter of generator names must be capitalized, that sounds a bit clearer.

Done in commit 7373486.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nicokosi I believe @navarroaxel is right about the extents of the token syntax; "value: " is your own custom text and there fore should be highlighted as needed. https://github.com/tldr-pages/tldr/pull/5675/files#r606702953

Copy link
Contributor

@bl-ue bl-ue Apr 4, 2021

Choose a reason for hiding this comment

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

Actually, I see that there's an issue here. fakedata has a special syntax for their templates where you enclose the capitalized template name in {{ and }}. Both the capitalization and the braces are required because fakedata uses the Go template library. The issue with that is that we at tldr-pages use that very syntax for a different purpose: that of highlighting user-entered values.

Instead of the original text here and @navarroaxel's suggestion, the appropriate would be this:

echo "{{value: {{Generator}}}}" | fakedata`

However, many clients do a blind replace of {{ and }} with HTML/ANSI escape codes that highlight the text, so we might need some sort of escaping, e.g.

echo "{{value: \{\{Generator\}\}}}" | fakedata`

But I'm just not sure. @sbrl what should we do here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, I see that there's an issue here. fakedata has a special syntax for their templates where you enclose the capitalized template name in {{ and }}. Both the capitalization and the braces are required because fakedata uses the Go template library. The issue with that is that we at tldr-pages use that very syntax for a different purpose: that of highlighting user-entered values.

Instead of the original text here and @navarroaxel's suggestion, the appropriate would be this:

echo "{{value: {{Generator}}}}" | fakedata`

However, many clients do a blind replace of {{ and }} with HTML/ANSI escape codes that highlight the text, so we might need some sort of escaping, e.g.

echo "{{value: \{\{Generator\}\}}}" | fakedata`

Done in 8ce9022.

pages/common/fakedata.md Outdated Show resolved Hide resolved
@sbrl
Copy link
Member

sbrl commented Apr 3, 2021

Left a comment here @bl-ue: #5675 (comment)

@nicokosi

This comment has been minimized.

@nicokosi
Copy link
Collaborator Author

nicokosi commented Apr 4, 2021

Left a comment here @bl-ue: #5675 (comment)

Done in 1c0bcc0.

@bl-ue
Copy link
Contributor

bl-ue commented Apr 4, 2021

Thank you @nicokosi. I left a new comment here: #5675 (comment)

pages/common/fakedata.md Outdated Show resolved Hide resolved
Co-authored-by: bl-ue <54780737+bl-ue@users.noreply.github.com>
Copy link
Contributor

@bl-ue bl-ue left a comment

Choose a reason for hiding this comment

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

Looks good to me. I want to know what @sbrl has to say regarding the syntax in the last example. Ping @sbrl

@nicokosi
Copy link
Collaborator Author

nicokosi commented Apr 8, 2021

Seems OK for me. @sbrl, still any blockers on your side?


- Generate data using a custom output template (the first letter of generator names must be capitalized):

`echo "{{\{\{Generator\}\}}}" | fakedata`
Copy link
Member

@sbrl sbrl Apr 8, 2021

Choose a reason for hiding this comment

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

Oh wow, this is a very awkward situation here. We don't actually define any standard way to escape this in the client spec. This is the only thing we mention:

Although this specification is about the interface that clients must provide, it is also worth noting that pages are written in standard CommonMark, which the exception of the non-standard {{ and }} syntax, which surrounds values in an example that users may edit. Clients MUST NOT break if the page format is changed within the CommonMark specification.

So the escaping with the backward slashes won't be handled correctly here. What happens if you remove the backwards slashes? How do clients handle that? I would assume that since the syntax can't be nested they would display it?

I think a client spec update is in order to properly define our non-standard token syntax.

Sorry to drag the review process out!

Copy link
Contributor

Choose a reason for hiding this comment

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

Also @sbrl some clients just blindly replace {{ and }} with ANSI codes/HTML to highlight, e.g. hidroh/tldroid (though that client it no longer maintained and we're going to remove it soon I think, #4044)

@navarroaxel navarroaxel merged commit 6b14fd4 into tldr-pages:master Apr 10, 2021
@bl-ue
Copy link
Contributor

bl-ue commented Apr 11, 2021

@navarroaxel I'm not sure this was supposed to be merged actually, see our conversation above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new command Issues requesting creation of a new page.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants