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

docs: fix the table display issue in metrics.md #14116

Merged
merged 2 commits into from
Jan 28, 2025
Merged

Conversation

jswxstw
Copy link
Member

@jswxstw jswxstw commented Jan 23, 2025

Motivation

Fix the table display issue in metrics.md

Clipboard_Screenshot_1737622204

Modifications

Verification

@jswxstw jswxstw requested a review from Joibel January 23, 2025 08:51
@jswxstw
Copy link
Member Author

jswxstw commented Jan 23, 2025

go run ./util/telemetry/builder --metricsDocs $@

@Joibel Please take a look at this when you have time. This will cause the tables in the documentation to not display correctly, which also makes docs ci task failure.

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

I wonder if we can catch this kind of issues with markdown linter

@jswxstw
Copy link
Member Author

jswxstw commented Jan 24, 2025

I wonder if we can catch this kind of issues with markdown linter

Markdownlint does not have a rule to recognize this type of case.
MkDocs uses a Markdown parser that strictly adheres to the CommonMark specification by default, while other parsers like GitHub Flavored Markdown (GFM) do not.

@Joibel Joibel self-assigned this Jan 24, 2025
@Joibel
Copy link
Member

Joibel commented Jan 24, 2025

Thanks for noticing this @jswxstw. I've fixed the generator to do the correct thing with table breaks, and also made it double space the "No attributes" lines for consistency.

I can't trivially make the generator layout the lines with ⚠️ correctly, it should really done with a fix to https://github.com/nao1215/markdown, but I don't think it's worth the effort. Let me know if you disagree.

@Joibel Joibel merged commit 67f675f into argoproj:main Jan 28, 2025
31 checks passed
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.

3 participants