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

pre-commit: Add markdownlint support #4222

Merged
merged 6 commits into from
Aug 19, 2021

Conversation

Holzhaus
Copy link
Member

This prevents introducing broken markdown.

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Please update the comments in .pre-commit-config.yam. You now also have to install Ruby in addition to Python for running the pre-commit hooks.

@Holzhaus
Copy link
Member Author

You now also have to install Ruby in addition to Python for running the pre-commit hooks.

Are you sure? According to https://pre-commit.com/#ruby, ruby hooks work without any system level dependencies. It should download a suitable ruby installation automatically.

The list is redundant as the hooks can be found by reading the rest of
the file and it prone to become outdated.
@uklotzde
Copy link
Contributor

uklotzde commented Aug 18, 2021

You now also have to install Ruby in addition to Python for running the pre-commit hooks.

Are you sure? According to https://pre-commit.com/#ruby, ruby hooks work without any system level dependencies. It should download a suitable ruby installation automatically.

The pre-commit hook failed until I manually installed rubygem.

@uklotzde
Copy link
Contributor

[INFO] Installing environment for https://github.com/markdownlint/markdownlint.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('gem', 'build', 'mdl.gemspec')
return code: 1
expected return code: 0
stdout:
    Executable `gem` not found
stderr: (none)

@Holzhaus
Copy link
Member Author

[INFO] Installing environment for https://github.com/markdownlint/markdownlint.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('gem', 'build', 'mdl.gemspec')
return code: 1
expected return code: 0
stdout:
    Executable `gem` not found
stderr: (none)

This looks like you have an incomplete ruby version installed. According to the code, it should only use the system ruby if you have both ruby and gem installed. Let me double check, maybe a bug in pre-commit.

@Holzhaus
Copy link
Member Author

@uklotzde please retest, it should now always install ruby regardless of the system.

@uklotzde
Copy link
Contributor

None of the Ruby stuff was installed (ruby/gem/rake).

The first check took ages and failed again:

[INFO] Installing environment for https://github.com/markdownlint/markdownlint.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('/usr/bin/bash', '/home/uk/.cache/pre-commit/repor6re3vs4/rbenv-2.6.6/bin/rbenv', 'install', '2.6.6')
return code: 1
expected return code: 0
stdout: (none)
stderr:
    Downloading ruby-2.6.6.tar.bz2...
    -> https://cache.ruby-lang.org/pub/ruby/2.6/ruby-2.6.6.tar.bz2
    Installing ruby-2.6.6...
    
    BUILD FAILED (Fedora 34 using ruby-build 20210510)
    
    Inspect or clean up the working tree at /tmp/ruby-build.20210818170924.121021.2GsUAL
    Results logged to /tmp/ruby-build.20210818170924.121021.log
    
    Last 10 log lines:
    installing capi-docs:               /home/uk/.cache/pre-commit/repor6re3vs4/rbenv-2.6.6/versions/2.6.6/share/doc/ruby
    The Ruby readline extension was not compiled.
    ERROR: Ruby install aborted due to missing extensions
    Try running `yum install -y readline-devel` to fetch missing dependencies.
    
    Configure options used:
      --prefix=/home/uk/.cache/pre-commit/repor6re3vs4/rbenv-2.6.6/versions/2.6.6
      --enable-shared
      LDFLAGS=-L/home/uk/.cache/pre-commit/repor6re3vs4/rbenv-2.6.6/versions/2.6.6/lib 
      CPPFLAGS=-I/home/uk/.cache/pre-commit/repor6re3vs4/rbenv-2.6.6/versions/2.6.6/include 
    
Check the log at /home/uk/.cache/pre-commit/pre-commit.log

@uklotzde
Copy link
Contributor

@Holzhaus Holzhaus force-pushed the pre-commit-markdownlint branch from ef9ea0e to 054d373 Compare August 18, 2021 17:39
@Holzhaus
Copy link
Member Author

I opened a bug report upstream: pre-commit/pre-commit#2016

@uklotzde For now I switched to a nodejs-based markdown linter. Please retest.

@Holzhaus Holzhaus requested a review from uklotzde August 18, 2021 19:27
@uklotzde
Copy link
Contributor

Nice side-effect: If you already use his markdownlint VS Code plugin then the configuration is recognized and warnings are also shown there.

@uklotzde
Copy link
Contributor

I still get warnings in CONTRIBUTING.md:

SKIP=end-of-file-fixer,trailing-whitespace,clang-format,eslint,no-commit-to-branch \
pre-commit run --all-files

@Holzhaus
Copy link
Member Author

I still get warnings in CONTRIBUTING.md:

SKIP=end-of-file-fixer,trailing-whitespace,clang-format,eslint,no-commit-to-branch \
pre-commit run --all-files

Weird. I don't get any warnings. Maybe it also uses some global config from vscode?

@Holzhaus
Copy link
Member Author

I still get warnings in CONTRIBUTING.md:

SKIP=end-of-file-fixer,trailing-whitespace,clang-format,eslint,no-commit-to-branch \
pre-commit run --all-files

Weird. I don't get any warnings. Maybe it also uses some global config from vscode?

No, CONTRIBUTING.md does not exist in my tree, because this PR targets the 2.3 branch.

@uklotzde
Copy link
Contributor

I still get warnings in CONTRIBUTING.md:

SKIP=end-of-file-fixer,trailing-whitespace,clang-format,eslint,no-commit-to-branch \
pre-commit run --all-files

Weird. I don't get any warnings. Maybe it also uses some global config from vscode?

No, CONTRIBUTING.md does not exist in my tree, because this PR targets the 2.3 branch.

My bad, tested on main.

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

@uklotzde uklotzde merged commit 89b742c into mixxxdj:2.3 Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants