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

Create initial workflow for markdown tests #64

Merged
merged 26 commits into from
Oct 14, 2020
Merged

Create initial workflow for markdown tests #64

merged 26 commits into from
Oct 14, 2020

Conversation

MichaIng
Copy link
Owner

@MichaIng MichaIng requested review from fpetru and StephanStS October 12, 2020 16:50
Seems globs must still be quotes
Update actions branch to current dev
@MichaIng
Copy link
Owner Author

MichaIng commented Oct 12, 2020

Okay this works. Syntax is fine now, the checks run through. Results are:

README.md: 1: MD041/first-line-heading/first-line-h1 First line in file should be a top level heading [Context: "Official Website: [dietpi.com]..."]
README.md: 5: MD013/line-length Line length [Expected: 80; Actual: 101]
README.md: 15: MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 4]
README.md: 15: MD013/line-length Line length [Expected: 80; Actual: 265]
README.md: 17: MD022/blanks-around-headings/blanks-around-headers Headings should be surrounded by blank lines [Expected: 1; Actual: 0; Below] [Context: "## License"]
README.md: 18: MD013/line-length Line length [Expected: 80; Actual: 383]
README.md: 20: MD013/line-length Line length [Expected: 80; Actual: 141]
README.md: 26: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
README.md: 26: MD013/line-length Line length [Expected: 80; Actual: 120]
README.md: 27: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
README.md: 27: MD013/line-length Line length [Expected: 80; Actual: 132]
README.md: 28: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
README.md: 32: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
README.md: 33: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
README.md: 34: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
README.md: 35: MD004/ul-style Unordered list style [Expected: dash; Actual: asterisk]
README.md: 39: MD034/no-bare-urls Bare URL used [Context: "https://github.com/MichaIng/Di..."]
README.md: 40: MD034/no-bare-urls Bare URL used [Context: "https://github.com/MichaIng/Di..."]
README.md: 41: MD034/no-bare-urls Bare URL used [Context: "https://github.com/MichaIng/Di..."]

Only README.md is scanned so far. Not sure if it breaks on the first file with errors or if my files glob does not span across sub directories. Will find that out.

  • EDIT: Jep needed to use a certain syntax to include really all markdown files.

You see it enforces certain style standards, string max line length, consistent list syntax etc. Most make sense, IMO, the 80 characters per line should be the limit for a documentation page that might be seen on 4k wide screen monitors is a different question. HTML line breaks automatically, so this would be something to disable via the actions config file.

  • EDIT: markdownlink configuration added to disable this rule and specify how we declare headers.

The rules are default markdownlint rules: https://github.com/DavidAnson/markdownlint/blob/main/doc/Rules.md
This is similar to Markdown what shellcheck is for shell/bash.

So e.g. I didn't understand what problems it has with the last three lines and MD034, so I check the list: https://github.com/DavidAnson/markdownlint/blob/main/doc/Rules.md#md034---bare-url-used
Okay for some reason adding URLs without any surrounding is not seen as good practice for Markdown, probably because not all Markdown readers handle those the same way, so we should add angle brackets for that 👍.


Implementing this the first time will for sure raise a large number of "errors", so it should be gone through these and decided whether those make sense in our case or we want to exclude them in our case, like the 80 character per line rule (IMO).

@fpetru
Copy link
Collaborator

fpetru commented Oct 13, 2020

@MichaIng - The idea is excellent - before we commit, we need to make successful the checks. Let me review the rules too and start applying the changes.

@MichaIng
Copy link
Owner Author

Jep, I'm still playing around, thinking about merging the tests together to avoid overhead and switching spellcheck to be done on the final html files instead of the initial markdown files, where it as well can be customised a bid better to e.g. exclude code. Link checking as well needs some solution to not check the same host too often and receive 429 response: raviqqe/liche#42

But there are already some invalid/outdated link finds, some actual typos and when we apply wrap links into angle brackets, those should be excluded from spellcheck, which covers most of the false alarms there. So basically we can merge before starting to solve things and then do that step by step instead.

@MichaIng
Copy link
Owner Author

Spellcheck action: https://github.com/rojopolis/spellcheck-github-actions
Uses PySpelling: https://facelessuser.github.io/pyspelling/configuration/

Link checker action: https://github.com/peter-evans/link-checker
Uses liche: https://github.com/raviqqe/liche
Since this must be done on the final html build, to allow checking the relative links between the pages, the docs themselves need to be build as well, which is anyway another reasonable test.

@MichaIng MichaIng marked this pull request as draft October 13, 2020 22:25
@fpetru
Copy link
Collaborator

fpetru commented Oct 14, 2020

@MichaIng - to be able to work on it, I plan to merge the pull request and start focusing first on check_spelling.

@fpetru fpetru marked this pull request as ready for review October 14, 2020 19:29
@fpetru fpetru merged commit afcb8f9 into dev Oct 14, 2020
@MichaIng MichaIng deleted the actions branch October 14, 2020 19:36
@MichaIng
Copy link
Owner Author

@fpetru
I'll change it now the way that it checks spelling on the final HTML files. PySpelling only reasonably checks Markdown files when converting them to HTML, which is what all the default configs do. But since we convert them ourself to HTML anyway, it makes much more sense to convert them out way, via MkDocs and do the spell checking on those, as the result might be a bid different. Also then we know exactly which text is wrapped in what kind of HTML tag and can create filters to e.g. no check code etc.

@MichaIng
Copy link
Owner Author

MichaIng commented Oct 14, 2020

@fpetru
Another hint when merging pull requests: Please select "Squash and merge" to merge the PR as a single commit.

I tend to write/develop on GitHub directly, hence there are a lot of commits, back and forth, typos, syntax etc. This messes the commit history unnecessary, so having them as a single clean final commit enhances beauty and readability of the commit history. @StephanStS same for you, as long as there is no good reason to have each commit separated => "Squash and merge"

Ah, and respect when the PR opener marks it as "draft" which basically means, "I'm still working on it", so do not merge until opener marked it to be ready 😉.

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