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

Add CMake linter cmake-lint #4036

Merged
merged 2 commits into from
Feb 6, 2022

Conversation

carlsmedstad
Copy link
Contributor

Add support for the CMake linter cmake-lint provided by https://github.com/cheshirekow/cmake_format.

A bit unsure about the naming due to the conflict with the other linter for CMake.

Resolves #3396.

@offa
Copy link
Contributor

offa commented Jan 21, 2022

Since the current linter is unmaintained, wouldn't it make sense to replace it with the new one?

@carlsmedstad
Copy link
Contributor Author

The old linter has been forked and now lives in cmake-lint/cmake-lint. Development is far from active, but I asked if the project was still maintained and the answer was yes.

So I don't think it's fair to replace it really.

@offa
Copy link
Contributor

offa commented Jan 21, 2022

Ah, thank you. I only knew the original repo.

Copy link
Contributor

@hsanson hsanson left a comment

Choose a reason for hiding this comment

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

Thanks, looks good. Just missing tests for the linter itself. Can you add one similar to test/linter/test_ruby.vader? It tests if the correct default command is returned and if the options are properly set if configured.

Add support for the CMake linter provided by
https://github.com/cheshirekow/cmake_format.
@carlsmedstad
Copy link
Contributor Author

Added the test and also wrapped the executable in ale#Escape() as I noticed that was the case for most other linters.

Copy link
Contributor

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

@hsanson hsanson merged commit c9938bc into dense-analysis:master Feb 6, 2022
@hsanson
Copy link
Contributor

hsanson commented Feb 6, 2022

@carlsmedstad @offa does this new linter replaces the current one available in ALE:

Wondering if we should consider removing it. I see from that repo that there has not been any activity in the last 5 years. But that alone is not indiciation that the linter is not being used.

@carlsmedstad
Copy link
Contributor Author

See my comment above for my opinion about that:

The old linter has been forked and now lives in cmake-lint/cmake-lint. Development is far from active, but I asked if the project was still maintained and the answer was cmake-lint/cmake-lint#8 (comment).

So I don't think it's fair to replace it really.

I think we should update the URL to point to the new repository at least (https://github.com/cmake-lint/cmake-lint).

@carlsmedstad carlsmedstad deleted the add-linter-cmake-lint branch February 6, 2022 12:45
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.

cmake-format also provides cmake-lint
3 participants