Skip to content

fix(linting): Select CMake files for linting using an include rather than exclude list; Finish incomplete changes from previous PRs. #11

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.

Sign up for GitHub

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

Merged
kirkrodrigues merged 3 commits into from
Oct 27, 2024
Merged

fix(linting): Select CMake files for linting using an include rather than exclude list; Finish incomplete changes from previous PRs. #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eaf1246
fix: Rename remaining reference to Taskfile.yml.
kirkrodrigues Oct 27, 2024
048ce2c
Remove unused .yamllint config.
kirkrodrigues Oct 27, 2024
951403f
Select CMake files for linting using an include rather than exclude l…
kirkrodrigues Oct 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions .yamllint.yaml
View file
Open in desktop

This file was deleted.

3 changes: 3 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ task lint:cpp-configs
## Adding files
Certain file types need to be added to our linting rules manually:

* **CMake**. If adding a CMake file, add it (or its parent directory) as an argument to the
`gersemi` command in [lint-tasks.yaml](lint-tasks.yaml).
* If adding a directory, the file must be named `CMakeLists.txt` or use the `.cmake` extension.
Comment on lines +22 to +24

@coderabbitai coderabbitai Bot Oct 27, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance clarity and consistency of CMake linting instructions

The current instructions could be clearer about the include-list approach. Consider these improvements:

-* **CMake**. If adding a CMake file, add it (or its parent directory) as an argument to the
-  `gersemi` command in [lint-tasks.yaml](lint-tasks.yaml).
-  * If adding a directory, the file must be named `CMakeLists.txt` or use the `.cmake` extension.
+* **CMake**. The project uses an include-list approach for CMake linting. When adding a new CMake file:
+  * Add the file path or its parent directory as an argument to the `gersemi` command in `lint-tasks.yaml`
+  * Files must be named either `CMakeLists.txt` or use the `.cmake` extension

The suggested changes:

  • Explicitly mention the include-list approach
  • Use consistent file referencing (remove square brackets)
  • Improve readability with better bullet point structure
  • Make the requirements more prominent
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
* **CMake**. If adding a CMake file, add it (or its parent directory) as an argument to the
`gersemi` command in [lint-tasks.yaml](lint-tasks.yaml).
* If adding a directory, the file must be named `CMakeLists.txt` or use the `.cmake` extension.
* **CMake**. The project uses an include-list approach for CMake linting. When adding a new CMake file:
* Add the file path or its parent directory as an argument to the `gersemi` command in `lint-tasks.yaml`
* Files must be named either `CMakeLists.txt` or use the `.cmake` extension

* **YAML**. If adding a YAML file (regardless of its extension), add it as an argument to the
`yamllint` command in [lint-tasks.yaml](lint-tasks.yaml).

Expand Down
9 changes: 2 additions & 7 deletions lint-tasks.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ tasks:
- "{{.G_SPIDER_CMAKE_CACHE}}"
- "{{.G_SPIDER_COMPILE_COMMANDS_DB}}"
- "{{.TASKFILE}}"
- "Taskfile.yml"
- "taskfile.yaml"
- "tools/yscope-dev-utils/lint-configs/.clang-tidy"
deps: [":config-cmake-project", "cpp-configs", "venv"]
cmds:
Expand All @@ -104,7 +104,6 @@ tasks:
--strict \
.gersemirc \
.github/ \
.yamllint.yaml \
lint-tasks.yaml \
taskfile.yaml

Expand Down Expand Up @@ -138,11 +137,7 @@ tasks:
vars: ["FLAGS"]
cmd: |-
. "{{.G_LINT_VENV_DIR}}/bin/activate"
find . \
-path ./build -prune \
-o -name CMakeLists.txt \
-print0 | \
xargs -0 --no-run-if-empty gersemi {{.FLAGS}}
gersemi {{.FLAGS}} CMakeLists.txt src/

venv:
internal: true
Expand Down