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

markdownlint-climarkdownlint-cli2 #1934

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

Conversation

parkerbxyz
Copy link
Contributor

@parkerbxyz parkerbxyz commented Oct 5, 2022

Proposed Changes

Replace markdownlint-cli with markdownlint-cli2.

Motivation: MegaLinter doesn't currently play nice with directory-specific markdownlint configuration files. Using markdownlint-cli2 in place of markdownlint-cli will fix this.

Advantages:

  • Speed (allegedly)
  • Configuration files can live anywhere and automatically apply to their part of the directory tree
    The markdownlint VS Code extension is powered by markdownlint-cli2, so everything should just work without special configuration when using the extension in conjunction with MegaLinter.
  • Fewer dependencies

Additional information: https://dlaa.me/blog/post/markdownlintcli2

Readiness Checklist

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

@parkerbxyz parkerbxyz marked this pull request as ready for review October 22, 2022 00:31
@parkerbxyz parkerbxyz requested a review from nvuillam as a code owner October 22, 2022 00:31
@parkerbxyz
Copy link
Contributor Author

The only markdownlint test that I see failing is this one:

megalinter/tests/test_megalinter/linters/markdown_markdownlint_test.py::markdown_markdownlint_test::test_failure FAILED [ 46%]

I haven't been able to find any logs indicating why it's failing and I haven't figured out how to run the tests locally to debug further. Any help here would be greatly appreciated. 🙂

@nvuillam
Copy link
Member

Maybe you can start y what are the advatages og megalinter to use maskdownlint-cli2 instead of markdownlint ? :)

@parkerbxyz
Copy link
Contributor Author

Maybe you can start y what are the advatages og megalinter to use maskdownlint-cli2 instead of markdownlint ? :)

I'm not sure I understand the question... If you're asking what the advantages of markdownlint-cli2 are over the original markdownlint CLI, I addressed that in the PR description. Let me know if there's something I should elaborate on there or if you have any other questions. 🙂

@Kurt-von-Laven
Copy link
Collaborator

The only markdownlint test that I see failing is this one:

megalinter/tests/test_megalinter/linters/markdown_markdownlint_test.py::markdown_markdownlint_test::test_failure FAILED [ 46%]

I haven't been able to find any logs indicating why it's failing and I haven't figured out how to run the tests locally to debug further. Any help here would be greatly appreciated. 🙂

I think this is very valid feedback. I am inclined to give our CI pipeline some love if @nvuillam agrees. I am open to suggestions on where to begin. Otherwise, I will just try to keep the PRs small in order to solicit feedback.

@nvuillam
Copy link
Member

image

There are errors in bad testing files
They are not detected by markdownlint-cli2, that's why CI job fails

@parkerbxyz
Copy link
Contributor Author

It looks like the test is adding markdownlint-cli2 as an argument to the command, like so:

markdownlint-cli2 markdownlint-cli2 /tmp/lint/.automation/test/markdown/markdown_bad_1.md /tmp/lint/.automation/test/markdown/markdown_bad_2.md

This appears to be because of how the container works. More on that here. If we want to use the container, we'll need to use the --entrypoint flag to indicate which command to run without adding it twice.

I'm going to see if the build success without the container and go from there.

@nvuillam
Copy link
Member

nvuillam commented Oct 23, 2022

I think using npm package could be ok :) (all npm packages are joined in a single install command so it won't impact badly the build time)

@parkerbxyz
Copy link
Contributor Author

parkerbxyz commented Oct 24, 2022

megalinter/tests/test_megalinter/helpers/utilstest.py:604: in assert_file_has_been_updated
    test_self.assertTrue(updated, f"{file_name} has been updated")
E   AssertionError: False is not true : markdown_for_fixes_1.md has been updated

If I run the following command locally on this branch, 17 files, including markdown_for_fixes_1.md get updated/fixed:

npx mega-linter-runner --path .automation/test/sample_project_fixes/ --fix

Any ideas why it might behave differently in the test?

@github-actions github-actions bot removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Feb 28, 2025
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.

4 participants