Escape pipe characters in asciidoc descriptions by margocrawf · Pull Request #17 · elastic/crd-ref-docs · GitHub
Skip to content

Escape pipe characters in asciidoc descriptions#17

Merged
thbkrkr merged 3 commits into
elastic:masterfrom
margocrawf:master
Jan 24, 2022
Merged

Escape pipe characters in asciidoc descriptions#17
thbkrkr merged 3 commits into
elastic:masterfrom
margocrawf:master

Conversation

@margocrawf

@margocrawf margocrawf commented Dec 1, 2021

Copy link
Copy Markdown
Contributor
  • Provides a function to escape the pipe character, which has special meaning for asciidoc as a way to format tables, so that including | in a comment does not result in wonky tables.
  • Wrap type_members in the new escape function

This does not include:

  • changes to the markdown renderer (since escaping could be flavor-specific. For example GFM would use \| but other flavors could use |).
  • Escaping other characters, although that could be added.

For context, my team uses crd-ref-docs, and it has worked great for the most part. However we want to describe an LDAP query with a pipe character in a comment, but this led to the API docs showing a table that didn't render correctly. See here

@margocrawf
Escape pipe characters in asciidoc descriptions
98b568e 
Signed-off-by: Margo Crawford <margaretc@vmware.com>
@cla-checker-service

cla-checker-service Bot commented Dec 1, 2021 •
edited
Loading

Copy link
Copy Markdown

@thbkrkr thbkrkr added the enhancement New feature or request label Dec 2, 2021
@thbkrkr

thbkrkr commented Dec 2, 2021

Copy link
Copy Markdown
Contributor

Did you consider escaping the pipe in the source comment?

@margocrawf

margocrawf commented Dec 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

That would make reading the comment in a code editor a worse experience. It's not a problem with the comment, it's a problem with the comment's interaction with markdown, so I'd rather isolate the solution to markdown.
It would also mess up kubectl explain, since we use the same code comment to generate those descriptions. I don't want kubectl explain activedirectoryidentityprovider.spec.userSearch to show a backslash* character.

thbkrkr
thbkrkr reviewed Dec 6, 2021
View reviewed changes
Comment thread test/api/v1/guestbook_types.go Outdated
@margocrawf margocrawf mentioned this pull request Dec 6, 2021
Closed
@thbkrkr

thbkrkr commented Dec 7, 2021

Copy link
Copy Markdown
Contributor

Ok, makes sense.

Could you please sign the CLA and we can merge it in?

@margocrawf

margocrawf commented Jan 3, 2022

Copy link
Copy Markdown
Contributor Author

Is there a way to re-trigger the CLA check? I asked VMware to add me to the CLA and they said they have emailed Elastic about being added, but it's all opaque to me.

@thbkrkr

thbkrkr commented Jan 4, 2022

Copy link
Copy Markdown
Contributor

cla/check

@gtback

gtback commented Jan 11, 2022

Copy link
Copy Markdown
Member

👋🏻 Hi, thanks for contributing @margocrawf! And sorry for the delay on the CLA. I found the email message (it went to an unmonitored email address rather than the one we use for CLA updates); we'll reply to that email and I'll update here once I've been able to add you to the list of approved CLAs.

@gtback

gtback commented Jan 19, 2022

Copy link
Copy Markdown
Member

cla/check

@gtback

gtback commented Jan 19, 2022

Copy link
Copy Markdown
Member

thbkrkr
thbkrkr reviewed Jan 24, 2022
View reviewed changes
Comment thread renderer/asciidoctor.go Outdated
Comment thread renderer/asciidoctor.go Outdated
Comment thread templates/asciidoctor/type_members.tpl Outdated
@margocrawf
Renamed escape function to renderfielddoc,
f96e5e1 
also added a comment and change parsing to use strings library
thbkrkr
thbkrkr approved these changes Jan 24, 2022
View reviewed changes

@thbkrkr thbkrkr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@thbkrkr thbkrkr merged commit efa87ff into elastic:master Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thbkrkr thbkrkr thbkrkr approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@margocrawf @thbkrkr @gtback