feat: CI - Add code rules checking action #3914
Conversation
arng40
added
type: CI
MelReyCG
changed the title
MelReyCG
changed the title
MelReyCG
left a comment
MelReyCG
left a comment
There was a problem hiding this comment.
I think you showed that script works, but it seems that there are some critical issues in it.
scripts/check_code_rules.sh
Outdated
| print_violation "$MAP_VIOLATIONS_FOUND" ARRAY_MAP "std::map" | ||
| print_violation "$UMAP_VIOLATIONS_FOUND" ARRAY_UMAP "std::unordered_map" | ||
| print_violation "$VECTOR_VIOLATIONS_FOUND" ARRAY_VECTOR "std::vector" |
There was a problem hiding this comment.
- Add an array for them,
- did you missed
std::array?
There was a problem hiding this comment.
array still not in GEOS
There was a problem hiding this comment.
OK
would you mind adding an array for forbidden strings? as it is you have dupplicated strings + repetitive code.
| --test-code-style | ||
| --test-documentation | ||
| -h | --help | ||
| -h | --help |
There was a problem hiding this comment.
maybe reversible whitespace edition for PR's clarity sake
.github/workflows/ci_tests.yml
Outdated
| - are_submodules_in_sync | ||
| - check_code_style_and_documentation | ||
| - cpu_builds | ||
| - check_code_standards |
There was a problem hiding this comment.
If check_code_standards is just rename of check_code_style_and_documentation why ?
And why check_code_rules is not required here ?
There was a problem hiding this comment.
Looking at CI checks, it is indeed then an advisory but not required enforcement ?
.github/workflows/ci_tests.yml
Outdated
| needs.check_code_style_and_documentation.result == 'success' && | ||
| needs.cpu_builds.result == 'success' && | ||
| needs.cuda_builds.result == 'success' && | ||
| needs.check_code_standards.result == 'success' && |
There was a problem hiding this comment.
was need.cpu_builds.results and its cuda counterpart removed while dev to speed up or is it permanent ??
scripts/check_code_rules.sh
Outdated
| "mesh/*" | ||
| "physicsSolvers/*" | ||
| ) | ||
| EXCLUDE_PATTERNS=( |
| - name: Check the code rules | ||
| run: "scripts/check_code_rules.sh" |
There was a problem hiding this comment.
Why is it no longer grouped with the code style & docs?
There was a problem hiding this comment.
it's not executed the same way as code style & docs
scripts/check_code_rules.sh
Outdated
| ## III. MAIN LOOP | ||
| ################################ | ||
|
|
||
| for file in ${ARRAY_FILES[@]}; do |
There was a problem hiding this comment.
IIUC, this array can have spaces too, doesn't it need "?
| fi | ||
| done | ||
|
|
||
| EXCLUDE_EXPRESSION=() |
There was a problem hiding this comment.
I'm confused, does it excludes?
This PR aim to create a new CI Target in order to ensure the use of the internal tool in geos and to promote the best coding practice.
First, this PR set up the CI
check_code_rulesand will ensure the use of :stdMap,stdUnorderedMap&stdVectorThis PR will be merged after :