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 markdownlint in CI and pre-commit hook #1890

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

zmostafa
Copy link
Contributor

@zmostafa zmostafa commented Feb 13, 2023

Signed-off-by: Ziad Mostafa ziad.mostafa@apex.ai

Pre-Review Checklist for the PR Author

  1. Code follows the coding style of CONTRIBUTING.md
  2. Tests follow the best practice for testing
  3. Changelog updated in the unreleased section including API breaking changes
  4. Branch follows the naming format (iox-123-this-is-a-branch)
  5. Commits messages are according to this guideline
  6. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  7. Relevant issues are linked
  8. Add sensible notes for the reviewer
  9. All checks have passed (except task-list-completed)
  10. All touched (C/C++) source code files from iceoryx_hoofs are added to ./clang-tidy-diff-scans.txt
  11. Assign PR to reviewer

Notes for Reviewer

Checklist for the PR Reviewer

  • Commits are properly organized and messages are according to the guideline
  • Code according to our coding style and naming conventions
  • Unit tests have been written for new behavior
  • Public API changes are documented via doxygen
  • Copyright owner are updated in the changed files
  • All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

@zmostafa zmostafa marked this pull request as draft February 13, 2023 14:46
@zmostafa
Copy link
Contributor Author

@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:

  • Force fixing the errors found by the linter using --fix flag
  • Analyze the error log and break the build when errors are still there.

Am I still missing something ?

@mossmaurice mossmaurice added documentation Improvements or additions to documentation tooling All iceoryx related tooling (scripts etc.) labels Feb 13, 2023
@zmostafa zmostafa force-pushed the iox-921-add-markdown-linter branch 2 times, most recently from 0c3cb9d to ec4217a Compare February 13, 2023 19:00
Copy link
Contributor

@mossmaurice mossmaurice left a 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

Comment on lines 63 to 67
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
Copy link
Contributor

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)
Copy link
Contributor

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?

Copy link
Contributor

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 <ziad.mostafa@apex.ai>
@zmostafa zmostafa force-pushed the iox-921-add-markdown-linter branch from ec4217a to 403271d Compare May 3, 2023 12:21
Signed-off-by: Ziad Mostafa <ziad.mostafa@apex.ai>
Signed-off-by: Ziad Mostafa <ziad.mostafa@apex.ai>
Signed-off-by: Ziad Mostafa <ziad.mostafa@apex.ai>
Signed-off-by: Ziad Mostafa <ziad.mostafa@apex.ai>
@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Merging #1890 (374c367) into master (90996dc) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 374c367 differs from pull request most recent head 709bea8. Consider uploading reports for the commit 709bea8 to get more accurate results

Impacted file tree graph

@@            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     
Flag Coverage Δ
unittests 73.72% <ø> (-0.01%) ⬇️
unittests_timing 14.73% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

@zmostafa zmostafa marked this pull request as ready for review May 4, 2023 12:43
@zmostafa zmostafa requested a review from mossmaurice May 4, 2023 12:43
@elBoberido
Copy link
Member

@zmostafa are you planing to continue working on this PR?

@zmostafa
Copy link
Contributor Author

@mossmaurice Do we still need the markdown linter? We concluded that we can postpone this since we also postponed the release.

cc. @elBoberido

@mossmaurice
Copy link
Contributor

mossmaurice commented Aug 3, 2023

@mossmaurice Do we still need the markdown linter? We concluded that we can postpone this since we also postponed the release.

@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.

@elBoberido
Copy link
Member

elBoberido commented Aug 3, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation tooling All iceoryx related tooling (scripts etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add markdown linter in CI
4 participants