feature: support for clang-format#402
Merged
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces clang-format support to the platform by adding builder functions and platform targets for code formatting, configurable through platformio.ini, with automatic package installation and optional local .clang-format file support. Changes
Sequence DiagramsequenceDiagram
participant Config as platformio.ini
participant Platform as platform.py
participant Installer as Package Installer
participant Builder as builder/main.py
participant ClangFmt as clang-format Tool
Config->>Platform: Read clang_format enabled flag
activate Platform
Platform->>Platform: _configure_clang_format()
Platform->>Installer: Install tool-clang-format package
deactivate Platform
activate Installer
Installer->>ClangFmt: Download & extract v21.1.0
deactivate Installer
Note over Builder: User invokes clangformat/clangformat-write target
activate Builder
Builder->>Builder: _clang_format_run(force_mode=check/write)
Builder->>Config: Read formatting options<br/>(dirs, extensions, args)
Builder->>Builder: Collect source files from configured directories
Builder->>ClangFmt: Execute clang-format with command
Builder->>Builder: Report results & handle errors
deactivate Builder
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)✅ Unit Tests committed locally.
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 |
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
builder/main.py (1)
1969-2011: Consider batching file arguments to avoid OS command-line limits.Large projects can exceed Windows’ command line length limit when passing all files at once. Splitting into batches improves reliability.
♻️ Example batching approach
- cmd.extend(source_files) + def _chunks(seq, size): + for i in range(0, len(seq), size): + yield seq[i:i + size] @@ - result = subprocess.run(cmd, check=False, capture_output=False) + rc = 0 + for batch in _chunks(source_files, 200): + result = subprocess.run(cmd + batch, check=False, capture_output=False) + rc = result.returncode + if rc != 0 and mode != "write": + break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/main.py` around lines 1969 - 2011, The current code appends all filenames from source_files into a single cmd which can exceed OS command-line length limits on large projects; change it to batch the files: after building the base cmd (using clang_format_bin, style/file, -i or --dry-run/--Werror, extra_args and CLI args from sys.argv) split source_files into manageable chunks (e.g., by fixed chunk size or by total argument length) and for each chunk construct batch_cmd = base_cmd + chunk and invoke the formatter per batch (using the existing subprocess invocation logic), preserving prints and failing fast on non-zero return codes so errors propagate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builder/main.py`:
- Around line 1933-2027: The clang-format task currently prints errors but
always returns None (SCons sees success); update the function to return proper
exit codes: return a non-zero int when the package or binary is missing or when
no source files were found (e.g., return 1), and after running subprocess.run
return result.returncode so SCons can fail on formatting errors; adjust the
FileNotFoundError and generic Exception handlers to return non-zero as well.
Locate the logic around clang_format_pkg, clang_format_bin, source_files, mode
and the subprocess.run call (result) and ensure each early-return path and each
exception path returns an integer error code instead of None, and the normal
completion returns result.returncode (or 0 on success).
In `@platform.py`:
- Around line 720-725: The _configure_clang_format method currently treats any
non-empty variables["clang_format"] as enabled (so "false" still triggers
install); update it to treat only explicit truthy values: read clang_format =
variables.get("clang_format", "") and normalize it (str(lower).strip()) or
accept actual booleans/ints, then check against an allow-list such as
{"1","true","yes","on"} (or bool(value) when value is already a bool) before
iterating FORMAT_PACKAGES and calling self.install_tool(package); leave behavior
unchanged for missing/empty values and ensure case-insensitive matching.
---
Nitpick comments:
In `@builder/main.py`:
- Around line 1969-2011: The current code appends all filenames from
source_files into a single cmd which can exceed OS command-line length limits on
large projects; change it to batch the files: after building the base cmd (using
clang_format_bin, style/file, -i or --dry-run/--Werror, extra_args and CLI args
from sys.argv) split source_files into manageable chunks (e.g., by fixed chunk
size or by total argument length) and for each chunk construct batch_cmd =
base_cmd + chunk and invoke the formatter per batch (using the existing
subprocess invocation logic), preserving prints and failing fast on non-zero
return codes so errors propagate.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
builder/main.py (1)
1986-1988:⚠️ Potential issue | 🟡 MinorTreat empty source set as success.
Failing the target when no files are found makes
clangformatnoisy in header‑only or empty projects. Consider returning success instead.🔧 Proposed fix
if not source_files: print("No source files found for clang-format.") - return 1 + return 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/main.py` around lines 1986 - 1988, The current check treats an empty source_files set as a failure by returning 1; change this to treat it as success by returning 0 (or otherwise exiting with success). Locate the branch that uses the source_files variable and prints "No source files found for clang-format." and replace the failing return value with a success return (e.g., return 0) so clangformat does not mark header-only or empty projects as failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@builder/main.py`:
- Around line 1986-1988: The current check treats an empty source_files set as a
failure by returning 1; change this to treat it as success by returning 0 (or
otherwise exiting with success). Locate the branch that uses the source_files
variable and prints "No source files found for clang-format." and replace the
failing return value with a success return (e.g., return 0) so clangformat does
not mark header-only or empty projects as failed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
builder/main.pyplatform.py
🚧 Files skipped from review as they are similar to previous changes (1)
- platform.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
builder/main.py (1)
1919-1919: Silence unused SCons action args to keep linters quiet.
Ruff flagstarget/sourceas unused; a small rename keeps the signature while avoiding warnings.♻️ Proposed tweak
-def _clang_format_run(target, source, env, force_mode=None): +def _clang_format_run(_target, _source, env, force_mode=None):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/main.py` at line 1919, The SCons action function _clang_format_run currently declares unused parameters target and source which trip linters; rename them (e.g., _target and _source or unused_target and unused_source) in the _clang_format_run signature to preserve the SCons API while silencing Ruff/linters and leave the remaining parameters/env/force_mode untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@builder/main.py`:
- Line 1919: The SCons action function _clang_format_run currently declares
unused parameters target and source which trip linters; rename them (e.g.,
_target and _source or unused_target and unused_source) in the _clang_format_run
signature to preserve the SCons API while silencing Ruff/linters and leave the
remaining parameters/env/force_mode untouched.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
platform.py (1)
529-529: Ambiguous EN DASH in comment.The comment uses
–(U+2013 EN DASH) instead of-(U+002D HYPHEN-MINUS). Ruff flags this as RUF003.✏️ Proposed fix
- # Case 3: Package not yet downloaded – fetch skeleton then install + # Case 3: Package not yet downloaded - fetch skeleton then install🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform.py` at line 529, Replace the EN DASH in the comment "Case 3: Package not yet downloaded – fetch skeleton then install" with a regular hyphen-minus so Ruff no longer flags RUF003; locate that comment (the "Case 3: Package not yet downloaded – fetch skeleton then install" line in platform.py) and change the U+2013 character to U+002D (i.e., "Case 3: Package not yet downloaded - fetch skeleton then install").builder/main.py (3)
2037-2039: Narrow the broadExceptioncatch.The generic
except Exception(Ruff BLE001) masks unexpected failures. The only realistic non-FileNotFoundErrorOS-level errors fromsubprocess.runareOSError/PermissionError.✏️ Proposed refactor
- except Exception as e: - print(f"Error running clang-format: {e}") - return 1 + except OSError as e: + print(f"Error running clang-format: {e}") + return 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/main.py` around lines 2037 - 2039, Narrow the broad except in the clang-format invocation: instead of catching Exception in the except block that handles subprocess.run, handle FileNotFoundError separately and then OSError/PermissionError for OS-level failures (e.g., use two except clauses targeting FileNotFoundError and PermissionError/OSError) and keep the existing logging and return behavior; reference the subprocess.run call and the current except Exception as e handler in builder/main.py to locate the lines to change.
1940-1940: Ambiguous EN DASH in comment.Same RUF003 issue as in
platform.py—–should be-.✏️ Proposed fix
- # Resolve clang-format executable from platform package – install on demand + # Resolve clang-format executable from platform package - install on demand🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/main.py` at line 1940, Replace the ambiguous EN DASH in the comment string "Resolve clang-format executable from platform package – install on demand" with a normal hyphen so it reads "Resolve clang-format executable from platform package - install on demand"; update the comment in builder/main.py accordingly to avoid the RUF003 lint issue and ensure only ASCII hyphen-minus is used.
1941-1949: RedundantPath()wrap on an already-Pathobject.
platform.packages_dir(seeplatform.py:232-237) already returns aPath, soPath(clang_format_pkg)on line 1951 is a no-op. The construction here is fine, but the downstream wrapping is unnecessary.✏️ Proposed fix
- clang_format_bin = Path(clang_format_pkg) / "clang-format" + clang_format_bin = clang_format_pkg / "clang-format"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/main.py` around lines 1941 - 1949, The code redundantly wraps an already-Path object with Path(...); locate uses of clang_format_pkg (set via clang_format_pkg = platform.packages_dir / "tool-clang-format") and remove any unnecessary Path(clang_format_pkg) wrapping—use clang_format_pkg directly and, if needed, assert or document that platform.packages_dir returns a pathlib.Path; update checks/install logic around platform.install_tool to operate on the Path object without re-wrapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@builder/main.py`:
- Around line 2037-2039: Narrow the broad except in the clang-format invocation:
instead of catching Exception in the except block that handles subprocess.run,
handle FileNotFoundError separately and then OSError/PermissionError for
OS-level failures (e.g., use two except clauses targeting FileNotFoundError and
PermissionError/OSError) and keep the existing logging and return behavior;
reference the subprocess.run call and the current except Exception as e handler
in builder/main.py to locate the lines to change.
- Line 1940: Replace the ambiguous EN DASH in the comment string "Resolve
clang-format executable from platform package – install on demand" with a normal
hyphen so it reads "Resolve clang-format executable from platform package -
install on demand"; update the comment in builder/main.py accordingly to avoid
the RUF003 lint issue and ensure only ASCII hyphen-minus is used.
- Around line 1941-1949: The code redundantly wraps an already-Path object with
Path(...); locate uses of clang_format_pkg (set via clang_format_pkg =
platform.packages_dir / "tool-clang-format") and remove any unnecessary
Path(clang_format_pkg) wrapping—use clang_format_pkg directly and, if needed,
assert or document that platform.packages_dir returns a pathlib.Path; update
checks/install logic around platform.install_tool to operate on the Path object
without re-wrapping.
In `@platform.py`:
- Line 529: Replace the EN DASH in the comment "Case 3: Package not yet
downloaded – fetch skeleton then install" with a regular hyphen-minus so Ruff no
longer flags RUF003; locate that comment (the "Case 3: Package not yet
downloaded – fetch skeleton then install" line in platform.py) and change the
U+2013 character to U+002D (i.e., "Case 3: Package not yet downloaded - fetch
skeleton then install").
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
platform.py (1)
731-742: LGTM!The explicit
bool → int → strdispatch with an allow-list allowset correctly addresses the previously flagged truthy-value issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform.py` around lines 731 - 742, The _configure_clang_format implementation correctly normalizes the variables["clang_format"] value (handling bool, int, and string allow-list) and installs each package from FORMAT_PACKAGES via install_tool when enabled, so no code changes are required; keep the _configure_clang_format function as-is and proceed with approval.
🧹 Nitpick comments (2)
builder/main.py (2)
2022-2039: Early-exit on batch failure skips remaining batches inwritemode.If any batch returns non-zero in
writemode (e.g., a clang-format crash), subsequent file batches are left unformatted without notice. This is low probability since-irarely fails mid-run, but it's worth documenting or adding a log message when bailing early in write mode.♻️ Suggested improvement for write-mode early exit
if result.returncode != 0: if mode != "write": print("clang-format: formatting issues found (see above).") else: + print(f"clang-format: batch {i // _CLANG_FORMAT_BATCH_SIZE + 1} failed, " + f"remaining files were not formatted.") print(f"clang-format exited with code {result.returncode}") return result.returncode🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/main.py` around lines 2022 - 2039, The loop over source_files using _CLANG_FORMAT_BATCH_SIZE may return early on any batch failure; update the handling inside the for-loop where result.returncode != 0 (in builder/main.py, referencing source_files, _CLANG_FORMAT_BATCH_SIZE, base_cmd and mode) to log a clear warning when mode == "write" before returning, e.g., note that clang-format failed partway and that remaining files may be unformatted (include result.returncode and clang_format_bin in the message), so callers know why formatting stopped and which executable failed.
1951-1951: RedundantPath()wrap —clang_format_pkgis already aPath.♻️ Proposed cleanup
- clang_format_bin = Path(clang_format_pkg) / "clang-format" + clang_format_bin = clang_format_pkg / "clang-format"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/main.py` at line 1951, The code wraps an object already typed as a Path with Path() when computing clang_format_bin; remove the redundant Path() call and set clang_format_bin = clang_format_pkg / "clang-format" so the result remains a Path. Update any nearby usages expecting a Path accordingly (references: clang_format_bin and clang_format_pkg).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builder/main.py`:
- Line 1940: Replace the EN DASH in the comment string "Resolve clang-format
executable from platform package – install on demand" with a standard ASCII
hyphen-minus so it reads "Resolve clang-format executable from platform package
- install on demand"; locate this comment in builder/main.py (search for the
exact comment text) and update the punctuation character only to satisfy RUF003.
- Line 1926: Update the docstring that documents the default value for
clang_format_dirs to match the actual default (remove "lib" so it lists only
"src" and "include"); locate the docstring or help text referencing
clang_format_dirs in builder/main.py (search for the identifier
"clang_format_dirs" and the surrounding help/comment block) and change the
described default directories from ["src", "include", "lib"] to ["src",
"include"] so it matches the runtime default set for clang_format_dirs.
In `@platform.py`:
- Line 529: Replace the EN DASH character in the comment "Case 3: Package not
yet downloaded – fetch skeleton then install" with a standard hyphen-minus (-)
to satisfy Ruff RUF003; locate that comment (the "Case 3: Package not yet
downloaded – fetch skeleton then install" line) in platform.py and edit the dash
character only, keeping the rest of the comment unchanged.
---
Duplicate comments:
In `@platform.py`:
- Around line 731-742: The _configure_clang_format implementation correctly
normalizes the variables["clang_format"] value (handling bool, int, and string
allow-list) and installs each package from FORMAT_PACKAGES via install_tool when
enabled, so no code changes are required; keep the _configure_clang_format
function as-is and proceed with approval.
---
Nitpick comments:
In `@builder/main.py`:
- Around line 2022-2039: The loop over source_files using
_CLANG_FORMAT_BATCH_SIZE may return early on any batch failure; update the
handling inside the for-loop where result.returncode != 0 (in builder/main.py,
referencing source_files, _CLANG_FORMAT_BATCH_SIZE, base_cmd and mode) to log a
clear warning when mode == "write" before returning, e.g., note that
clang-format failed partway and that remaining files may be unformatted (include
result.returncode and clang_format_bin in the message), so callers know why
formatting stopped and which executable failed.
- Line 1951: The code wraps an object already typed as a Path with Path() when
computing clang_format_bin; remove the redundant Path() call and set
clang_format_bin = clang_format_pkg / "clang-format" so the result remains a
Path. Update any nearby usages expecting a Path accordingly (references:
clang_format_bin and clang_format_pkg).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
builder/main.py (1)
1919-1919: Silence Ruff ARG001 for unused SCons parameters.
targetandsourceare unused; if Ruff is enforced, prefix with_to avoid warnings.♻️ Proposed fix
-def _clang_format_run(target, source, env, force_mode=None): +def _clang_format_run(_target, _source, env, force_mode=None):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/main.py` at line 1919, The function _clang_format_run currently has unused parameters target and source which trigger Ruff ARG001; rename them to _target and _source in the function signature (leave env and force_mode as-is) so the linter treats them as intentionally unused; update any internal references (if any) to use the new names or remove references, ensuring the function body compiles and behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builder/main.py`:
- Around line 2025-2027: The subprocess.run call using base_cmd + batch (the
variables base_cmd and batch) accepts user-supplied args and triggers Ruff S603;
fix by either validating/whitelisting the elements of batch before invoking
subprocess.run (ensure only allowed clang-format flags/filenames are present,
sanitize or rebuild the argv list) and prefer passing an explicit list (no
shell=True), or if you decide validation is infeasible, add an explicit inline
Ruff suppression on that invocation (e.g., noqa: S603) with a brief
justification comment. Locate the subprocess.run call in builder/main.py (the
code that builds base_cmd and batch) and implement the whitelist/sanitization
logic or add the documented suppression next to the subprocess.run call.
---
Nitpick comments:
In `@builder/main.py`:
- Line 1919: The function _clang_format_run currently has unused parameters
target and source which trigger Ruff ARG001; rename them to _target and _source
in the function signature (leave env and force_mode as-is) so the linter treats
them as intentionally unused; update any internal references (if any) to use the
new names or remove references, ensuring the function body compiles and behavior
is unchanged.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
builder/main.pyplatform.py
🚧 Files skipped from review as they are similar to previous changes (1)
- platform.py
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Unit tests committed locally. Commit: |
4 tasks
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Checklist:
Summary by CodeRabbit