-
Notifications
You must be signed in to change notification settings - Fork 397
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 markdownlint in CI and pre-commit hook #1890
base: main
Are you sure you want to change the base?
Enable markdownlint in CI and pre-commit hook #1890
Conversation
@mossmaurice @dkroenke Just a first commit for enabling the markdownlint tool in pre-commit and also add it to the docker image,did not enable the check now. Next steps:
Am I still missing something ? |
0c3cb9d
to
ec4217a
Compare
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.
Thanks for the PR @zmostafa, much appreciated.
Next steps:
- Force fixing the errors found by the linter using --fix flag
- Analyze the error log and break the build when errors are still there.
I think it would be good to right away fix the files here on this PR.
Am I still missing something?
What we should do is to try if it affects the MkDocs
build via tools/website/export-docu-to-website.sh local
. I think it won't have an effect. Anyway, IIRC the MkDocs
build is currently broken, but we should evaluate the output without the linter changes on this PR and with these changes.
cc @dkroenke
.markdownlint.yaml
Outdated
line_length: 100 | ||
# Number of characters for headings | ||
heading_line_length: 100 | ||
# Number of characters for code blocks | ||
code_block_line_length: 100 # Ignored due to next option |
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.
@zmostafa What do you think about setting the values to 120
like for the code via clang-format
?
cd "$(git rev-parse --show-toplevel)" | ||
|
||
if [[ "$SCOPE" == "hook"* ]]; then | ||
md_files=$(git diff --cached --name-only --diff-filter=ACMRT | grep -E "\.md"| cat) |
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 it's good to run the linter on all markdown files.
@elfenpiff What do you think?
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, it is actually very important!
Just think file A links to file B and one moves file B. Then the link of file A is broken but this will be never detected since only the modified files are checked and A was never modified.
Signed-off-by: Ziad Mostafa <[email protected]>
ec4217a
to
403271d
Compare
Signed-off-by: Ziad Mostafa <[email protected]>
Signed-off-by: Ziad Mostafa <[email protected]>
Signed-off-by: Ziad Mostafa <[email protected]>
Signed-off-by: Ziad Mostafa <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #1890 +/- ##
==========================================
- Coverage 74.05% 74.05% -0.01%
==========================================
Files 404 404
Lines 15924 15924
Branches 2243 2243
==========================================
- Hits 11793 11792 -1
Misses 3422 3422
- Partials 709 710 +1
Flags with carried forward coverage won't be shown. Click here to find out more. |
@zmostafa are you planing to continue working on this PR? |
@mossmaurice Do we still need the markdown linter? We concluded that we can postpone this since we also postponed the release. cc. @elBoberido |
@zmostafa Yes, the markdown linter would be much appreciated. As we decided to postpone the 3.0 release until new user- experienceable features will be added, this does not have a super high prio. |
@mossmaurice @zmostafa okay, it has not high priority but it also does not mean one can not work any time on this. I just wanted to be sure this PR does not stay open forever without the intention to continue to work on it. Take you time an finish it comfortably. |
Signed-off-by: Ziad Mostafa [email protected]
Pre-Review Checklist for the PR Author
iox-123-this-is-a-branch
)iox-#123 commit text
)task-list-completed
)iceoryx_hoofs
are added to./clang-tidy-diff-scans.txt
Notes for Reviewer
Checklist for the PR Reviewer
Code according to our coding style and naming conventionsUnit tests have been written for new behaviorPublic API changes are documented via doxygenAll touched (C/C++) source code files fromiceoryx_hoofs
have been added to./clang-tidy-diff-scans.txt
Post-review Checklist for the PR Author
References