-
-
Notifications
You must be signed in to change notification settings - Fork 150
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 checking of Markdown documents to our linters #116
Conversation
.github/workflows/analysis.yml
Outdated
sudo pip3 install pylint beautifulsoup4 html5lib | ||
sudo gem install mdl | ||
sudo chmod go-w /usr/share/rust/.cargo/bin |
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.
On startup, Ruby prints a warning about a permission issue in this directory - so we fix it up silence this unrelated and distracting warning.
@@ -0,0 +1 @@ | |||
rules "~MD005", "~MD033", "~MD046" |
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.
33 will complain about embedded HTML and didn't like <sup>
, so that had to go.
5 would easily get confused with nested ordered lists.
46 was too restrictive by enforcing only one way to do code presentation. This harmed the ascii-presentation of the markdown files when it makes sense to mix simple indentation lines with multi-line code blocks ( ```).
Also re-order to perform shellcheck first because it requires the least installation work compared to pylint and markdownlint. The reason being if we're going to fail during shellcheck, then we fail faster (and leave heavier tasks for further down the line).
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.
Overall very cool checker - I think only tiny adjustments are needed.
README.md
Outdated
<sup>*- SDL 1.2 was last updated 2013-08-17 and SDL\_sound 2008-04-20</sup> | ||
|
||
<sup>† - 22.05 kHz, 44.1 kHz, 48 kHz; mono, stereo</sup> | ||
|
||
<sup>‡ - 44.1 kHz stereo only</sup> | ||
|
||
<sup>§ - Broken or unsupported in either SDL\_sound or DOSBox</sup> |
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.
Previously these lines used 2 spaces to break the line, which I presumed was correct markdown… Does it generate any warning? In this case, we don't want every annotation to be in a separate paragraph.
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.
Yes, separate paragraphs is unnecessary visual bloat. Two (or more) spaces still generates:
- README.md:48: MD009 Trailing spaces
- README.md:49: MD009 Trailing spaces
- README.md:50: MD009 Trailing spaces
Edit: Interesting.. Markdown uses the more explicit "\" to create a line-break without starting a new paragraph, similar to HTML's <br>
. Will try that.
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.
Edit 2: \ works!
Makes me think we can now safely add a "strip-trailing-whitespace" commit-hook, because nothing I know of should have trailing whitespace.. can you think of anything?
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.
Nope, breaking space was the only thing - and the new way is better than having 2 trailing spaces :)
README.md
Outdated
#### Fedora | ||
|
||
$ sudo dnf install libpng SDL2 SDL2_net opusfile | ||
### Fedora |
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 I made an error earlier - sections Linux, Windows, macOS should be subsections of "Development snapshot builds", and this would keep sections per distro at level 4, otherwise it might be visually hard to distinguish where next section starts.
Change it to:
### [Linux](…)
…
#### Fedora
…
### [Windows](…)
…
This way linter probably won't complain about nesting structure.
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.
Will do!
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.
Yes, nice and consistent now.
README.md
Outdated
cd dosbox-staging | ||
./autogen.sh | ||
./configure | ||
make |
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.
Maybe shell syntax highlight? :)
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.
Will do - but I do like how tidy the raw text looked in your original version :-)
The linter didn't like the $ prefixes (they make it harder for users to copy and paste).
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.
Fenced, along with a couple others.
scripts/verify-markdown.sh
Outdated
# Based on `verify-bash.sh` by Patryk Obara <patryk.obara@gmail.com> | ||
# Copyright (C) 2019 licensed GPL-2.0-or-later |
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.
Eh, you can remove me - additional copyright line won't cause problems for scripts updating years in the future :).
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.
sounds good; yeah, was just trying to show attribution - but yeah, too much of that would get cluttered!
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.
Done.
Squashed and pushed. |
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.
Looks great, thanks :)
This PR uses mdl (markdownlint) which has become a popular checker for Markdown files.
Links to our three current Markdown docs: