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

Nonconforming syntax in multiple markdown files #792

Closed
scottamain opened this issue Dec 20, 2022 · 6 comments · Fixed by #793
Closed

Nonconforming syntax in multiple markdown files #792

scottamain opened this issue Dec 20, 2022 · 6 comments · Fixed by #793
Labels

Comments

@scottamain
Copy link
Contributor

What happened?

Although some MD parsers are pretty forgiving (including GitHub's), the current markdown files in stablehlo include a variety of syntax errors that do not conform to the basic MD syntax rules. Examples include improper indentation for lists, unescaped asterisks signs (meant for bold or italic text), and missing blank lines between headings and paragraphs.

We should follow traditional MD syntax rules to ensure well-structured files that are easy to read, use an agreed-upon structure that can be tested, and that are compatible with more than one MD parser.

The simplest solution is to markdownlint and fix various syntax and style issues.

We can submit fixes for individual files, and make exceptions as appropriate because this tool allows you to disable certain rules in the linter. Then, once the files are cleaned up, we should enable a GitHub Action to run markdownlint on all new pull requests with .md files so we can avoid re-introduction of syntax errors.

Steps to reproduce your issue

No response

Version information

No response

@burmako
Copy link
Contributor

burmako commented Dec 20, 2022

@scottamain Thank you for looking into this! This will be a nice improvement to the state of the art. We had prior discussions about Markdown linting but haven't had the time to follow up yet.

burmako pushed a commit that referenced this issue Dec 20, 2022
This is a first pass at some simple issues found by `markdownlint`, but
more will come as I gradually enable some of the lint rules that are
currently disabled. Also, fixing the spec will be a massive change so
that will be a separate PR perhaps done once the spec is near a more
final state.

Partially fixes #792
@burmako
Copy link
Contributor

burmako commented Dec 20, 2022

Reopening the ticket because #793 said that it only partially fixes it.

@burmako burmako reopened this Dec 20, 2022
@GleasonK
Copy link
Member

Not sure if this lint has been run on the rfcs/ directory as well? I didn't see the Compatibility RFC in #793, and I'm doubtful that I used perfect MD syntax all the way through.

Here are all md files in the repo.

burmako pushed a commit that referenced this issue Dec 21, 2022
More fixes for #792 with expanded scope to MD files not under /docs.
(Still not touching the spec yet.)

This includes markdownlint fixes that could not be fixed by
`markdownlint-cli`, such as line length (80 char), consistent list item
indications, proper heading levels, removal of bash "$" character when
not necessary
([rationale](https://cirosantilli.com/markdown-style-guide/#dollar-signs-in-shell-code);
if the snippet includes multiple commands, then add a line break).
GleasonK pushed a commit that referenced this issue Dec 21, 2022
As per #792, here's a script to run `markdownlint`. The bash script
includes usage instructions. You just need to pass it a file, directory,
or glob pattern for the MD files you want to lint. It uses a Docker
image because I fear the npm package version dependency hell, but the
docker image is versioned.
@scottamain
Copy link
Contributor Author

Hehe, yeah @GleasonK I didn't do the rfc files in the first CL just to keep my first PR simple. I'm sure you saw I've now cleaned those ones as well.

I think there's just two items remaining before closing this issue (in order):

  1. Fix all lint errors in spec.md. I'm happy to do this but I don't want to cause confusing merge conflicts for others because this appears to be a fairly active document. @burmako Should we wait a little longer for this?
  2. Enable the GitHub Action to run markdownlint on all .md file PRs.

@GleasonK
Copy link
Member

Should we wait a little longer for this?

Spoke with Eugene, totally fine to cause merge conflicts. There is only one open PR touching spec now, which makes this a good time to make that change. That plan sounds good to me.

Got the script running locally, looks like its only spec:

$ ./build_tools/github_actions/lint_markdown.sh "**/*.md" 2>&1 | grep -v spec
<no output, all failures are in spec.md>

GleasonK pushed a commit that referenced this issue Jan 19, 2023
GleasonK pushed a commit to GleasonK/stablehlo that referenced this issue Feb 10, 2023
@scottamain scottamain removed their assignment May 15, 2023
@burmako
Copy link
Contributor

burmako commented May 29, 2023

Given that we have a CI job now which lints markdown files, I think we can close this ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants