Support pre-commit and apply changes#20
Conversation
📝 WalkthroughWalkthroughThis change standardizes repository formatter configuration and pre-commit checks, reflows documentation, introduces header-guard tooling, renames migration include guards, and reformats Gauss hit finder implementations, registrations, serialization, and tests without reported behavioral changes. ChangesRepository formatting and migration cleanup
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.markdownlint.yaml:
- Around line 1-2: Replace the deprecated MD013 option `code_blocks` with its
supported replacement `ignore_code_blocks` in the markdownlint configuration.
In `@scripts/fix_header_guards.py`:
- Around line 30-31: Update check_header_guard so missing or incomplete guard
directives return an invalid result instead of (True, None), including the
short-file path and the corresponding checks around lines 52-53. Ensure fix mode
inserts the appropriate guard for guardless .h/.hpp files, or explicitly
excludes such files from validation.
- Around line 38-43: The scanner may rewrite unrelated conditionals because it
independently selects the first `#ifndef` and later `#define`. Update the
guard-detection logic used by the scanning and fix paths to require a leading,
structurally paired include guard: the first relevant directive must be `#ifndef`,
immediately followed by a matching `#define` for the same macro, with no
intervening directives. Only perform rewrites when this validated pair is found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4f4459d6-b4c3-41ba-ae99-3643f560a2d4
📒 Files selected for processing (29)
.clang-format.gersemirc.markdownlint.yaml.pre-commit-config.yamlREADME.mdmigration/doc/conf.pymigration/doc/index.rstmigration/gauss_hit_finder/CMakeLists.txtmigration/gauss_hit_finder/README.mdmigration/gauss_hit_finder/compare_hits.pymigration/gauss_hit_finder/copied_from_larsoft_minor_edits/.clang-formatmigration/gauss_hit_finder/find_hits_with_gaussians.cppmigration/gauss_hit_finder/find_hits_with_gaussians.hppmigration/gauss_hit_finder/find_hits_with_gaussians_design1.cppmigration/gauss_hit_finder/find_hits_with_gaussians_design1.hppmigration/gauss_hit_finder/print_hits_to_file.cppmigration/gauss_hit_finder/print_hits_to_file.hppmigration/gauss_hit_finder/register_find_hits_with_gaussians.cppmigration/gauss_hit_finder/register_find_hits_with_gaussians_cell_id.cppmigration/gauss_hit_finder/register_find_hits_with_gaussians_design1.cppmigration/gauss_hit_finder/register_print_hits_to_file.cppmigration/gauss_hit_finder/test_find_hits_with_gaussians.jsonnetmigration/gauss_hit_finder/test_find_hits_with_gaussians_design1.jsonnetmigration/gauss_hit_finder/wire_serialization.cppmigration/gauss_hit_finder/wire_serialization.hppmigration/gauss_hit_finder/wires_source.cppscripts/fix_header_guards.pysplit_square_verify.cppsubtract.py
💤 Files with no reviewable changes (1)
- migration/doc/index.rst
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-07-08T20:31:37.310Z
Learnt from: wddgit
Repo: Framework-R-D/phlex-examples PR: 18
File: migration/gauss_hit_finder/register_find_hits_with_gaussians_design1.cpp:49-63
Timestamp: 2026-07-08T20:31:37.310Z
Learning: In Framework-R-D/phlex-examples, files under `migration/` that are LArSoft-derived copies should be kept as close as possible to the original LArSoft implementation. During code review, avoid suggesting behavioral changes or extra “hardening” (e.g., new validation, altered logic, different edge-case handling) beyond what is strictly needed to support the phlex migration/demo or to make the code compile/integrate. If a deviation from upstream is required, keep it minimal and document the reason.
Applied to files:
migration/gauss_hit_finder/register_find_hits_with_gaussians_cell_id.cppmigration/gauss_hit_finder/wires_source.cppmigration/gauss_hit_finder/register_print_hits_to_file.cppmigration/gauss_hit_finder/register_find_hits_with_gaussians.cppmigration/gauss_hit_finder/print_hits_to_file.cppmigration/gauss_hit_finder/find_hits_with_gaussians_design1.cppmigration/gauss_hit_finder/wire_serialization.cppmigration/gauss_hit_finder/register_find_hits_with_gaussians_design1.cppmigration/gauss_hit_finder/find_hits_with_gaussians.cpp
🪛 ast-grep (0.44.1)
migration/doc/conf.py
[error] 19-19: Command coming from incoming request
Context: subprocess.check_output(["git", "rev-parse", "--short", "HEAD"])
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 19-19: Avoid command injection
Context: subprocess.check_output(["git", "rev-parse", "--short", "HEAD"])
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-python)
🪛 Clang (14.0.6)
migration/gauss_hit_finder/print_hits_to_file.cpp
[warning] 16-16: 2 adjacent parameters of 'print_hits_to_file' of similar type are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 16-16: the first parameter in the range is 'cell_id'
(clang)
[note] 16-16: the last parameter in the range is 'input_hits'
(clang)
[note] 16-16: 'int' and 'const int &' parameters accept and bind the same kind of values
(clang)
migration/gauss_hit_finder/find_hits_with_gaussians_design1.cpp
[warning] 42-42: constructor does not initialize these fields: begin_, end_
(cppcoreguidelines-pro-type-member-init)
[warning] 55-55: use a trailing return type for this function
(modernize-use-trailing-return-type)
migration/gauss_hit_finder/find_hits_with_gaussians.cpp
[warning] 24-24: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 24-24: parameter name 'x' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 26-26: statement should be inside braces
(readability-braces-around-statements)
[warning] 30-30: statement should be inside braces
(readability-braces-around-statements)
[warning] 33-33: statement should be inside braces
(readability-braces-around-statements)
[warning] 40-40: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 42-42: 2 adjacent parameters of 'find_hits_with_gaussians' of similar type ('const int &') are easily swapped by mistake
(bugprone-easily-swappable-parameters)
[note] 42-42: the first parameter in the range is 'wires'
(clang)
[note] 43-43: the last parameter in the range is 'cand_hit_standard'
(clang)
migration/gauss_hit_finder/find_hits_with_gaussians_design1.hpp
[warning] 81-81: use a trailing return type for this function
(modernize-use-trailing-return-type)
🪛 Cppcheck (2.21.0)
migration/gauss_hit_finder/wires_source.cpp
[style] 35-35: The function 'isValidChannelID' is never used.
(unusedFunction)
migration/gauss_hit_finder/print_hits_to_file.cpp
[style] 16-16: The function 'print_hits_to_file' is never used.
(unusedFunction)
migration/gauss_hit_finder/find_hits_with_gaussians_design1.cpp
[style] 55-55: The function 'predicate' is never used.
(unusedFunction)
migration/gauss_hit_finder/wire_serialization.cpp
[style] 35-35: The function 'isValidChannelID' is never used.
(unusedFunction)
[style] 124-124: The function 'to_int' is never used.
(unusedFunction)
[style] 6-6: The function 'write_wires_to_file' is never used.
(unusedFunction)
[style] 59-59: The function 'read_wires_from_file' is never used.
(unusedFunction)
migration/gauss_hit_finder/find_hits_with_gaussians.cpp
[style] 35-35: The function 'isValidChannelID' is never used.
(unusedFunction)
[style] 40-40: The function 'find_hits_with_gaussians' is never used.
(unusedFunction)
[style] 124-124: The function 'to_int' is never used.
(unusedFunction)
🪛 LanguageTool
migration/gauss_hit_finder/README.md
[style] ~95-~95: The expression “make sense” can be too colloquial for certain contexts. For a more formal tone, try using an alternative.
Context: ...lemented as a single phlex transform. Does it make sense to split it with unfolds and folds? Doe...
(WOULD_IT_MAKE_SENSE)
[style] ~96-~96: The expression “make sense” can be too colloquial for certain contexts. For a more formal tone, try using an alternative.
Context: ...nse to split it with unfolds and folds? Does it make sense to have multiple transforms? We don't k...
(WOULD_IT_MAKE_SENSE)
[grammar] ~180-~180: Use a hyphen to join words.
Context: ...mber for each wire. For now, I just hard coded the plane number to 0. The signal ...
(QB_NEW_EN_HYPHEN)
🪛 Ruff (0.15.20)
migration/doc/conf.py
[error] 16-16: Variable copyright is shadowing a Python builtin
(A001)
[error] 20-20: Starting a process with a partial executable path
(S607)
scripts/fix_header_guards.py
[warning] 105-105: Too many branches (14 > 12)
(PLR0912)
[warning] 126-127: Use elif instead of else then if, to reduce indentation
Convert to elif
(PLR5501)
[warning] 137-138: Use elif instead of else then if, to reduce indentation
Convert to elif
(PLR5501)
🔇 Additional comments (30)
.clang-format (1)
2-72: LGTM!Also applies to: 92-114, 126-133, 144-159, 168-190, 213-216, 226-226, 245-245, 255-276
.gersemirc (1)
1-13: LGTM!.pre-commit-config.yaml (2)
1-10: LGTM!Also applies to: 20-31, 39-60
12-19: 🩺 Stability & AvailabilityNo newline fix needed here. The header-guard hook only rewrites
.h/.hpp, and the tracked headers are already LF-only, so this hook order doesn’t create a mixed-ending conflict in this repo.> Likely an incorrect or invalid review comment.README.md (1)
14-21: LGTM!Also applies to: 99-100, 123-123
migration/doc/conf.py (1)
10-40: LGTM!Also applies to: 59-71
migration/gauss_hit_finder/README.md (1)
3-67: LGTM!Also applies to: 82-139, 160-220
migration/gauss_hit_finder/compare_hits.py (1)
6-6: LGTM!Also applies to: 16-19
migration/gauss_hit_finder/find_hits_with_gaussians.cpp (1)
18-46: LGTM!Also applies to: 87-87, 124-130, 141-141, 156-157, 196-197, 206-207, 259-259, 303-304, 337-382, 405-405, 425-425, 437-454
migration/gauss_hit_finder/find_hits_with_gaussians_design1.cpp (1)
18-19: LGTM!Also applies to: 42-42, 55-58
migration/gauss_hit_finder/wire_serialization.cpp (1)
1-5: LGTM!Also applies to: 6-57, 59-135
migration/gauss_hit_finder/wires_source.cpp (1)
26-35: LGTM!migration/gauss_hit_finder/CMakeLists.txt (1)
3-16: LGTM!Also applies to: 25-58, 70-98, 101-114, 115-155
migration/gauss_hit_finder/register_find_hits_with_gaussians_cell_id.cpp (1)
13-14: LGTM!split_square_verify.cpp (1)
35-47: LGTM!Also applies to: 48-50
migration/gauss_hit_finder/print_hits_to_file.cpp (1)
16-17: LGTM!Also applies to: 26-26, 39-39
migration/gauss_hit_finder/register_find_hits_with_gaussians.cpp (1)
15-16: LGTM!Also applies to: 34-34, 43-48, 57-77
migration/gauss_hit_finder/register_find_hits_with_gaussians_design1.cpp (1)
44-44: LGTM!Also applies to: 53-58, 67-96
migration/gauss_hit_finder/register_print_hits_to_file.cpp (1)
20-21: LGTM!migration/gauss_hit_finder/test_find_hits_with_gaussians.jsonnet (1)
36-36: LGTM!migration/gauss_hit_finder/test_find_hits_with_gaussians_design1.jsonnet (1)
37-37: LGTM!subtract.py (1)
7-7: LGTM!Also applies to: 40-42
migration/gauss_hit_finder/copied_from_larsoft_minor_edits/.clang-format (1)
1-1: LGTM!scripts/fix_header_guards.py (2)
1-24: LGTM!
105-143: 📐 Maintainability & Code QualityResolve the reported pre-commit lint diagnostics.
Ruff reports
PLR0912formainandPLR5501for bothelse/ifbranches. If those rules are enabled by the new pre-commit configuration, this script will fail the hook; refactor these branches or configure the rules explicitly.Source: Linters/SAST tools
migration/gauss_hit_finder/find_hits_with_gaussians.hpp (1)
1-2: LGTM!Also applies to: 46-61, 64-64
migration/gauss_hit_finder/find_hits_with_gaussians_design1.hpp (2)
1-2: LGTM!Also applies to: 71-79, 88-92
81-86: 📐 Maintainability & Code QualityVerify the clang-tidy policy for this declaration.
Clang reports
modernize-use-trailing-return-typehere. If clang-tidy is enforced by pre-commit, this header may still fail despite the formatting cleanup; apply the project-wide convention or disable/suppress the check consistently.Source: Linters/SAST tools
migration/gauss_hit_finder/print_hits_to_file.hpp (1)
1-13: LGTM!migration/gauss_hit_finder/wire_serialization.hpp (1)
1-14: LGTM!
| MD013: | ||
| code_blocks: false |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- .markdownlint.yaml ---\n'
cat -n .markdownlint.yaml
printf '\n--- markdownlint references ---\n'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' --glob '!coverage' 'markdownlint|markdownlint-cli|MD013|ignore_code_blocks|code_blocks' .Repository: Framework-R-D/phlex-examples
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- .markdownlint.yaml ---'
cat -n .markdownlint.yaml
echo
echo '--- markdownlint references ---'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' --glob '!coverage' 'markdownlint|markdownlint-cli|MD013|ignore_code_blocks|code_blocks' .
echo
echo '--- dependency manifests mentioning markdownlint ---'
git ls-files | rg '(^|/)(package\.json|package-lock\.json|pnpm-lock\.yaml|yarn\.lock|bun\.lockb|requirements\.txt|poetry\.lock|Gemfile.lock|go\.mod|Cargo\.toml)$' | while read -r f; do
if rg -n 'markdownlint' "$f" >/dev/null 2>&1; then
echo "### $f"
rg -n 'markdownlint' "$f"
fi
doneRepository: Framework-R-D/phlex-examples
Length of output: 542
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- .pre-commit-config.yaml (markdownlint section) ---'
nl -ba .pre-commit-config.yaml | sed -n '48,72p'Repository: Framework-R-D/phlex-examples
Length of output: 261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- .pre-commit-config.yaml (markdownlint section) ---'
cat -n .pre-commit-config.yaml | sed -n '48,72p'
echo
echo '--- markdownlint-cli2 version refs in repo ---'
rg -n 'markdownlint-cli2|markdownlint' .pre-commit-config.yaml .github .gitlab-ci.yml .circleci . 2>/dev/nullRepository: Framework-R-D/phlex-examples
Length of output: 973
🌐 Web query:
markdownlint MD013 code_blocks deprecated ignore_code_blocks markdownlint-cli2 v0.22.1
💡 Result:
The parameter code_blocks for rule MD013 has been deprecated in favor of ignore_code_blocks [1][2][3]. To align with consistent naming conventions across markdownlint rules, ignore_code_blocks was introduced as a more descriptive alternative [4][5]. When using the deprecated code_blocks parameter (specifically if set to false), the tool will output a deprecation warning advising you to replace it with ignore_code_blocks: true [1][2][3]. If you are using markdownlint-cli2 v0.22.1, it inherits this behavior because it relies on the underlying markdownlint library where these changes were implemented [6][7]. To update your configuration, change your MD013 settings as follows: Deprecated: "MD013": { "code_blocks": false } Recommended: "MD013": { "ignore_code_blocks": true } [1][2][3]
Citations:
- 1: markdownlint/markdownlint@2e48d4a
- 2: https://github.com/markdownlint/markdownlint/blob/a1662430ce84ac720dbba6e1c04846eaf6102449/CHANGELOG.md
- 3: https://github.com/markdownlint/markdownlint/blob/main/CHANGELOG.md
- 4: Introduce
ignore_code_blocksparameter to MD013 markdownlint/markdownlint#404 - 5: MD013: Rename parameter "code_blocks" markdownlint/markdownlint#398
- 6: https://github.com/DavidAnson/markdownlint-cli2/blob/main/CHANGELOG.md
- 7: DavidAnson/markdownlint-cli2@v0.22.0...v0.22.1
Use ignore_code_blocks for MD013
code_blocks is the deprecated form here; switch to the replacement to avoid noisy lint warnings going forward.
Suggested change
MD013:
- code_blocks: false
+ ignore_code_blocks: true📝 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.
| MD013: | |
| code_blocks: false | |
| MD013: | |
| ignore_code_blocks: true |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.markdownlint.yaml around lines 1 - 2, Replace the deprecated MD013 option
`code_blocks` with its supported replacement `ignore_code_blocks` in the
markdownlint configuration.
| if len(lines) < 3: | ||
| return True, None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not treat guardless headers as valid.
check_header_guard returns (True, None) when either directive is missing, so the pre-commit hook silently accepts unguarded .h/.hpp files; fix mode also declines to add a guard. Reject these files and either insert the guard or explicitly exclude guardless headers.
Also applies to: 52-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/fix_header_guards.py` around lines 30 - 31, Update check_header_guard
so missing or incomplete guard directives return an invalid result instead of
(True, None), including the short-file path and the corresponding checks around
lines 52-53. Ensure fix mode inserts the appropriate guard for guardless .h/.hpp
files, or explicitly excludes such files from validation.
| for i in range(min(10, len(lines))): | ||
| if m := re.match(r"#ifndef\s+(\w+)\s*$", lines[i]): | ||
| ifndef_idx, ifndef_macro = i, m.group(1) | ||
| elif m := re.match(r"#define\s+(\w+)\s*$", lines[i]): | ||
| define_idx, define_macro = i, m.group(1) | ||
| break |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Only rewrite a proven include-guard pair.
The scanner assumes the first #ifndef and first later #define are the guard directives. A header with an unrelated conditional in its first ten lines could therefore have that conditional rewritten by fix mode. Require a leading, structurally paired guard before modifying anything.
Also applies to: 73-78, 90-98
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/fix_header_guards.py` around lines 38 - 43, The scanner may rewrite
unrelated conditionals because it independently selects the first `#ifndef` and
later `#define`. Update the guard-detection logic used by the scanning and fix
paths to require a leading, structurally paired include guard: the first
relevant directive must be `#ifndef`, immediately followed by a matching `#define`
for the same macro, with no intervening directives. Only perform rewrites when
this validated pair is found.
1 participant
The
pre-commitconfiguration is very similar to that of https://github.com/Framework-R-D/phlex, with a few exceptions:migration/gauss_hit_finder/copied_from_larsoft_minor_editsis excluded frompre-commitandclang-tidyTooling & CI
scripts/fix_header_guards.pyto validate and repair C/C++ include guards..gersemircand updated MarkdownLint configuration to allow long lines in code blocks.Code quality
Build & tests
Documentation