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

enable: markdown lint + check: links + spelling #308

Merged
merged 48 commits into from
Sep 16, 2024
Merged

Conversation

hilmarf
Copy link
Member

@hilmarf hilmarf commented Aug 19, 2024

What this PR does / why we need it:

This better public relation, when all links are working and there are no typing errors.

Which issue(s) this PR fixes:

Some of the issues will be fixed as soon as open-component-model/ocm#871 is merged.

@hilmarf hilmarf added this to the 2024-Q3 milestone Aug 19, 2024
@hilmarf hilmarf requested a review from frewilhelm August 29, 2024 13:29
- code
- pre
sources:
- '**/*.md'
Copy link
Contributor

Choose a reason for hiding this comment

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

We also have html sources if I am not mistaken (e.g. https://github.com/open-component-model/ocm-website/blob/main/layouts/index.html). Would it be possible to check them? Or would the html-syntax be too troublesome.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not too convinced that the spell checker really makes sense for html pages and propose to keep the check for markdown only.

ocmcli
ocmconfig
oncefunc
oo
Copy link
Contributor

Choose a reason for hiding this comment

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

i grep -i'd for oo but did not find any occurrence. Are there any?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also didn't find any and guess oo can be removed

preconfigured
predefine
programmatically
prs
Copy link
Contributor

Choose a reason for hiding this comment

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

The occurrences of prs are PRs. Maybe we should just write pull-requests instead of adding prs to the wordlist to prevent any real typo of prs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although there' the chance of misspelling PRs as Pre or pre or any other permutation, I don't want to always write Pull-request instead of PRs and am willing to live with potential "errors" here :-)

.github/workflows/publish-site.yaml Outdated Show resolved Hide resolved
config/_default/hugo.toml Outdated Show resolved Hide resolved
content/docs/cli-reference/help/logging.md Show resolved Hide resolved
@@ -3,23 +3,20 @@ package main
import (
"bytes"
"fmt"
"github.com/spf13/cobra"
Copy link
Contributor

Choose a reason for hiding this comment

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

The import-order seems unintended.

@@ -4,13 +4,12 @@ import (
"flag"
"fmt"
"log"
clictx "ocm.software/ocm/api/cli"
Copy link
Contributor

Choose a reason for hiding this comment

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

the import-order seems unintended. Probably, an IDE setting or the like

@hilmarf hilmarf marked this pull request as ready for review September 16, 2024 07:36
@hilmarf hilmarf requested a review from frewilhelm September 16, 2024 07:37
@hilmarf hilmarf enabled auto-merge (squash) September 16, 2024 07:37
@hilmarf
Copy link
Member Author

hilmarf commented Sep 16, 2024

although spellchecks and md-lint is still failing, I would really like to merge this ASAP, because now we have NO broken links anymore ;-)

after other tasks, I can then maybe pickup the remaining open things in new PRs

@morri-son morri-son disabled auto-merge September 16, 2024 07:43
@morri-son morri-son enabled auto-merge (squash) September 16, 2024 07:43
@morri-son morri-son merged commit 19218cb into main Sep 16, 2024
1 of 3 checks passed
@morri-son morri-son deleted the verify/markdown branch September 16, 2024 07:43
* [ocm <b>sign</b>](/docs/cli-reference/sign) &mdash; Sign components or hashes
* [ocm <b>transfer</b>](/docs/cli-reference/transfer) &mdash; Transfer artifacts or components
* [ocm <b>verify</b>](/docs/cli-reference/verify) &mdash; Verify component version signatures
* [ocm <b>add</b>](/docs/cli-reference/add/) &mdash; Add elements to a component repository or component version
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we indent every line on the same height?

@ikhandamirov
Copy link
Contributor

@hilmarf, @morri-son, is this PR the reason why hundreds of checks are now failing in #323? Any recommendation?

@hilmarf
Copy link
Member Author

hilmarf commented Sep 16, 2024

@hilmarf, @morri-son, is this PR the reason why hundreds of checks are now failing in #323? Any recommendation?

yes, Lint Markdown + Spellcheck needs additional attention --> follow up PRs

for now, just ignore the failures or feel free to fix some of them ;-)

@morri-son
Copy link
Contributor

for the spellchecker I'm still not convinced that we really need it, as it focuses just on words in the dictionary, the rest, especially abbreviations, need to be entered in an exclusions file. For the markdown linter we should carefully check the default checks done and adopt it to our needs. The whole idea of any of the switched on checks in the end should be to add them as mandatory to pass and not to ignore them.

@frewilhelm
Copy link
Contributor

for the spellchecker I'm still not convinced that we really need it, as it focuses just on words in the dictionary, the rest, especially abbreviations, need to be entered in an exclusions file.

But less typos are a good thing, right? As we experienced it in the ocm repository, there were some actual typos and a brief look at the checks in Ilyas PR shows also some typos (and a lot of duplicate-mentiones that can be resolved rather quickly).

@morri-son
Copy link
Contributor

for the spellchecker I'm still not convinced that we really need it, as it focuses just on words in the dictionary, the rest, especially abbreviations, need to be entered in an exclusions file.

But less typos are a good thing, right? As we experienced it in the ocm repository, there were some actual typos and a brief look at the checks in Ilyas PR shows also some typos (and a lot of duplicate-mentiones that can be resolved rather quickly).

sure less typos are a good thing :-) And I'm not in general against a spell checker, but getting blocked by this as the checker brings up issues with "HCM" or "OCM" or "SAP" and forces to update the exclusion file is not really efficient. For sure, most-likely we will have all potential abbreviations and other terms that fit the whole topic maintained, but...

@frewilhelm
Copy link
Contributor

maybe there is a possibility to use the spellchecker as reusable workflow in the .github repository and having the wordlist also there for all our repositories. This would reduce the maintenance. However, if new abbreviations are introduced, one has to open another PR in the .github repository.

In general I would not make the spellchecker as required workflow. It is a nice to have, but should not block our development.

@hilmarf
Copy link
Member Author

hilmarf commented Sep 16, 2024

I also would like to have the spellchecker enabled but not blocking. It is valuable, although it's not perfect... open-component-model/ocm#928 maybe sooner or later some fancy AI github action is more able to also detect syntax and grammar issues. Or can even check that a new abbreviation has been introduced correctly before usage.

Currently none of the checks is a blocker. But I highly recommend to set the html-link-check as mandatory. Nothing is more frustrating than not working links, especially when they sound promising with your current problem ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔒Closed
Development

Successfully merging this pull request may close these issues.

Add GitHub action to check markdown files for broken links
4 participants