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

Add GitHub action checking links in .md files #48

Conversation

k3KAW8Pnf7mkmdSMPHz27
Copy link
Contributor

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 commented Mar 24, 2021

Adds a check for broken links when .md files are changed. It should probably ignore some files, hence the draft status.

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Contributor Author

k3KAW8Pnf7mkmdSMPHz27 commented Mar 24, 2021

Also, the latest run on my fork

I have started to fix all broken links that are found but it is going to take a while and will be in a different PR.

@themightychris
Copy link
Collaborator

@k3KAW8Pnf7mkmdSMPHz27 it would be awesome to have a markdown linter in this workflow (or a separate one?) too

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Contributor Author

@themightychris I'll look at adding a linter in the next couple of days (Friday most likely).

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Contributor Author

Well, expect an update on Friday,

  1. Check README.md and docs/
  2. Exclude some "URLs" that should not work
  3. Add linter

@k3KAW8Pnf7mkmdSMPHz27
Copy link
Contributor Author

k3KAW8Pnf7mkmdSMPHz27 commented Mar 26, 2021

I added a linter with some default options, but I think there is a need to look over them, see https://github.com/k3KAW8Pnf7mkmdSMPHz27/brigade-project-index/runs/2205386468?check_suite_focus=true for latest run.

(I don't have time to look at it further today but I'll get back to it)

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 marked this pull request as ready for review March 28, 2021 17:32
@k3KAW8Pnf7mkmdSMPHz27
Copy link
Contributor Author

@themightychris I'd argue that the rules are a bit on the strict side, but they are fairly easy to disable so perhaps keep them as is and see how it works out in the long run.

I'll start updating the markdown documents.

MD 001, 009, 022, and 047 all seem to strict for this project
@k3KAW8Pnf7mkmdSMPHz27
Copy link
Contributor Author

Additionally, disabling these might be a good idea,

  • MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]. I don't think this actually does anything.
  • MD034/no-bare-urls Bare URL used. I prefer [Meaningful name](http://link) to bare URLs, but perhaps it shouldn't be checked by the linter.

@themightychris
Copy link
Collaborator

I don't see any reason to keep consecutive blank lines around, I always clear those out to shut the linter up

I'd also leave the no-bare-urls rule on, bare URL's can simply be wrapped like <http://link> to turn them into links -- full links with meaningful names are ideal but when there isn't one or the content is the URL you can just wrap URLs in angle brackets

markdown-link-check-configuration.json Outdated Show resolved Hide resolved
{
"ignorePatterns": [
{
"pattern": "brigade-project-index/settings/secrets"
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this one just fail because it requires being logged in as an admin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd guess so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It exists if you have the right access if that is what you are asking 😛
There might be a point to not ignoring it, having a clickable link that only some people can use is not necessarily the best idea.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think ignoring it makes sense. We can document the system publically even if the public won't have access to every resource. If someone came around years from now after we're all gone and wanted to update things they'd be able to figure out what access to request from who from that information

@@ -0,0 +1,135 @@
default: false # includes/excludes all rules by default
Copy link
Collaborator

Choose a reason for hiding this comment

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

what all did you customize here? I usually just use the default markdownlint config with mkdocs sites, if you had to disable a lot of rules they're probably things we should just fix in the content

Copy link
Contributor Author

@k3KAW8Pnf7mkmdSMPHz27 k3KAW8Pnf7mkmdSMPHz27 Apr 21, 2021

Choose a reason for hiding this comment

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

E.g., rule 13 will be set to false, and I don't really see the point of enforcing it.

I view the purpose of the mkdoc generated page as an easy enough way for anyone to go in and make changes. If all the markdown rules are enforced with their default options, there will be quite a few warnings whenever someone not familiar with markdown makes changes. It is not a problem per se, but that is why I disable rules that don't necessarily add that much value and are likely to be violated.

I can set them all to true or switch to markdownlint/alternative configuration if you want.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I find it's best to stick to the default configuration as closely as possible, since a lot of editors with built-in linting will be following that same standard. It is better that contributors be mildly annoyed by the strict linter, than for built-in linting in editors to be useless because they're highlighting so many rules that we're ignoring. When the starting point for any contributor is that their editor will show zero errors or warnings when they open our markdown files, the warnings/errors that come up while they work will be easier to notice and deal with

Copy link
Collaborator

Choose a reason for hiding this comment

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

@k3KAW8Pnf7mkmdSMPHz27 I went ahead and merged this so we could see it in action, and created #53 as a followup

@themightychris themightychris merged commit e8e47f0 into civictechindex:master May 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.

2 participants