-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add GitHub action checking links in .md files #48
Conversation
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. |
@k3KAW8Pnf7mkmdSMPHz27 it would be awesome to have a markdown linter in this workflow (or a separate one?) too |
@themightychris I'll look at adding a linter in the next couple of days (Friday most likely). |
Well, expect an update on Friday,
|
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) |
@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
Additionally, disabling these might be a good idea,
|
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 |
{ | ||
"ignorePatterns": [ | ||
{ | ||
"pattern": "brigade-project-index/settings/secrets" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd guess so.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Adds a check for broken links when .md files are changed. It should probably ignore some files, hence the draft status.