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

Workflow for linting markdown files #129

Closed
wants to merge 6 commits into from

Conversation

wolf99
Copy link
Contributor

@wolf99 wolf99 commented Jan 9, 2023

Following from #100 this PR adds a workflow to lint any markdown files within the repository.

On #100 it was suggested to use Prettier for this. Instead I have used David Anson's markdownlint-cli2 action. There are a couple reasons not to use Prettier.

  • Prettier does not support Go, so would be adding a very broad tool for one simple part of functionality
  • Actions are simpler to use in GitHub workflows
  • I'm not too familiar with Prettier 😄

Developers still have the ability to run this check from a development environment using markdownlint-cli2 itself or any intergration in their IDE, such as this one for vscode.

That said, if there is a strong preference for Prettier I would be happy to look into using that instead 🙂 .

@wolf99
Copy link
Contributor Author

wolf99 commented Jan 9, 2023

Hmm, the lint action is not detecting the README.md file for some reason... I'll investigate

@wolf99 wolf99 marked this pull request as draft January 9, 2023 20:33
@G-Rath
Copy link
Collaborator

G-Rath commented Jan 9, 2023

I'm not familiar with markdownlint-cli2 so don't know how different it is by default vs prettier, but I'd say the two rank about the same in terms of boardness of tools since you can run it with npx prettier so long as Node is installed (see here for an example of how to set it up in GitHub Actions), and noting that prettier does support other files like YAML, JSON, etc which could be in this repo.

I think though there's no harm in trying to run both because prettier is focused on being a formatter, so won't catch stuff like broken links which I expect this linting tool to - also acknowledging I'm a guest on this project, so really it's up to @another-rex and @oliverchang who might feel differently :)

(really, the big thing I'm a fan with prettier on markdown files, aside from how easy it is to run and the general consistency factor, is proseWrap set to always because I find it fits nicely with terminals and other file lengths - afaik other tools don't support that, but markdownlint-cli2 might 🤷)

@wolf99
Copy link
Contributor Author

wolf99 commented Jan 9, 2023

Hi @G-Rath
markdownlint can be configured to fail if a given line lengths are exceeded.
Could you suggest what would be the acceptable line length? The old standard 80 char?

@G-Rath
Copy link
Collaborator

G-Rath commented Jan 9, 2023

@wolf99 how smart is markdownlint in applying that and does it support autofixing? most tools I've seen typically do line length checks by literally counting the number of chars, whereas prettier uses AST parsing meaning it understands situations where you can't actually split the content across multiple lines (i.e. such as with links and codeblocks).

Also does markdownlint support formatting codeblocks that have contents like JSON?

@wolf99
Copy link
Contributor Author

wolf99 commented Jan 9, 2023

Re auto-fixing: yes this is supported. Although I am not sure if that is advisable in a PR context as peoples work can get mutated without them realizing what's happened. A failure in a workflow forces them to check the flow output.

Re the line lengths: In short, yes 🙂
See: https://github.com/DavidAnson/markdownlint/blob/main/doc/md013.md

I haven't looked into how it does it, but it basically ignores certain cases such as tables, code blocks, etc.

Re formatting code within code blocks - no, this action formats markdown only. AFAIK it treat the contents of code blocks as code and ignores them (unless configured to fix things like line lengths specifically within them)

@G-Rath
Copy link
Collaborator

G-Rath commented Jan 9, 2023

Re auto-fixing: yes this is supported. Although I am not sure if that is advisable in a PR context as peoples work can get mutated without them realizing what's happened. A failure in a workflow forces them to check the flow output.

Agreed, but we want to make it easy for people to resolve the problems being flagged (especially ones that are tedious and trivial!) - otherwise it can make it harder for people to try and contribute🙂

@DavidAnson
Copy link

FYI, markdownlint and related tools do NOT try to fix line length issues. If that is important, you may want to look elsewhere.

Becuase they've *alread* got lint issues since PR google#100
@wolf99
Copy link
Contributor Author

wolf99 commented Jan 12, 2023

Ok - so this is now working as expected, except for fixing inline.
I am not sure if that is agreed as wanted?
Happy to add if it is.

If you prefer Prettier given the above input from David (Thanks David!), then I can open another PR for that?

@wolf99 wolf99 mentioned this pull request Jan 13, 2023
another-rex pushed a commit that referenced this pull request Dec 18, 2023
This uses `prettier` to format non-Go files using my (opinioned) config;
note that I'd recommend having `proseWrap` set to `always` but that has
a _huge_ number of changes so I'll do a seperate PR for that.

Resolves #144
Resolves #129
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