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

Markdown syntax: avoid a table here because it cannot handle pipes in cells #495

Merged
merged 2 commits into from
Aug 25, 2021

Conversation

trentm
Copy link
Member

@trentm trentm commented Aug 24, 2021

GitHub Flavored Markdown tables cannot cope with literal | (vertical bars) in cells data. They get interpreted as cell delimiters. That is a problem for the regexes in https://github.com/elastic/apm/blob/master/specs/agents/configuration.md#configuration-value-types

The regexes below are incomplete when rendered:

Type Description (if needed)
... ...
Duration ... Validating regex: `^(-)?(\d+)(ms
Size ... Validating regex: `^(\d+)(b

Option 1

Per https://stackoverflow.com/questions/49809122/vertical-bar-symbol-within-a-markdown-table one workaround is using &#124; and <code>...</code> like so:

| Type | Description (if needed) |
|------|-------------------------|
| ...  | ... |
| Duration | ... Validating regex: <code>^(-)?(\d+)(ms&#124;s&#124;m)$</code> |
| Size     | ... Validating regex: <code>^(\d+)(b&#124;kb&#124;mb&#124;gb)$</code> |

Rendered:

Type Description (if needed)
... ...
Duration ... Validating regex: ^(-)?(\d+)(ms|s|m)$
Size ... Validating regex: ^(\d+)(b|kb|mb|gb)$

The downside is that this regex is obscure for those reading the raw .md file in their editor.

Option 2

Use a list instead of a table.
Rendered: https://github.com/trentm/apm/blob/no-table-no-cry/specs/agents/configuration.md#configuration-value-types

@trentm trentm requested a review from basepi August 24, 2021 16:44
@trentm trentm self-assigned this Aug 24, 2021
@apmmachine
Copy link

apmmachine commented Aug 24, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-08-24T16:45:42.241+0000

  • Duration: 3 min 15 sec

  • Commit: 2bf4a56

Trends 🧪

Image of Build Times

@felixbarny
Copy link
Member

Either of the two options LGTM

Copy link
Contributor

@basepi basepi left a comment

Choose a reason for hiding this comment

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

I like the list, as shown in the current diff. Markdown is about source readability in addition to rendered readability.

@trentm trentm marked this pull request as ready for review August 25, 2021 18:25
@trentm trentm requested review from a team as code owners August 25, 2021 18:25
@trentm trentm removed request for a team August 25, 2021 18:26
@trentm trentm merged commit fb25fd1 into elastic:master Aug 25, 2021
@trentm trentm deleted the no-table-no-cry branch August 25, 2021 18:26
trentm added a commit to trentm/apm that referenced this pull request Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants