Page MenuHomePhabricator

[lintstaged] Add cmake-lint to lintstagedrc
Needs RevisionPublic

Authored by jon on Aug 9 2022, 9:49 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 16, 6:42 PM
Unknown Object (File)
Mon, Dec 16, 6:42 PM
Unknown Object (File)
Sun, Dec 15, 10:25 PM
Unknown Object (File)
Sun, Dec 1, 5:13 PM
Unknown Object (File)
Nov 14 2024, 5:10 PM
Unknown Object (File)
Nov 11 2024, 1:51 AM
Unknown Object (File)
Oct 28 2024, 5:08 AM
Unknown Object (File)
Oct 28 2024, 5:05 AM

Details

Reviewers
atul
Summary

Run cmake-lint as part of lintstaged

Test Plan

Install cmake-lint, or use nix develop

# open a CMakeLists.txt, and add a line more than 80 characters to the top of
# the file

git checkout -b tmp
git add <file>
git commit -m "test" # should fail

Diff Detail

Repository
rCOMM Comm
Branch
jonringer/cmake-lintstaged (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Not sure, but I think we should add a section on the docs to tell the user how to install cmake-lint, because otherwise they won't be able to commit code that's matched by the cmake-lint glob in .lintstagedrc.js. Do we need pip to install cmake-lint?

Something like D4771 (you can probably just copy the format in there) should be amended to this stack (probably before this diff), because this addition to lint-staged depends on the user having cmake-lint already installed. Also we'll have to send a message for everyone to install cmake-lint anyways, like @atul said in D4772.

Also, you can probably rename the [chore] part in this diff to something like [lint-staged] or [lintstaged] just going off prior examples like D4620 and D4772. Or you could make it [cmake-lint]. For me, [chore] is usually reserved for a nit or trivial change, not something like installing/adding a new dependency.

Other than that, this looks good! But requesting changes for your/other reviewers' thoughts on the above feedback.

This revision now requires changes to proceed.Aug 9 2022, 10:15 AM

Do we need pip to install cmake-lint?

Problem with asking people to do pip install cmakelang is that the package requirements will likely fail to be compatible with any other system installed package. Homebrew and nix get around this by creating per-package venv's but that's not available with raw pip invocations.

jon retitled this revision from [Chore] Add cmake-lint to lintstagedrc to [lintstaged] Add cmake-lint to lintstagedrc.Aug 9 2022, 2:59 PM

If we're fully deprecating non-Nix workflow we can probably add cmake-lint to Nix and bring back this diff?

This revision now requires review to proceed.Mar 12 2023, 4:08 PM
atul requested changes to this revision.Mar 12 2023, 4:08 PM
This revision now requires changes to proceed.Mar 12 2023, 4:08 PM