feat: get rid of toml and packaging and improve performance
#89
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #89 +/- ##
==========================================
- Coverage 97.01% 95.48% -1.53%
==========================================
Files 3 3
Lines 134 133 -1
==========================================
- Hits 130 127 -3
- Misses 4 6 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
""" WalkthroughThe Changes
Sequence Diagram(s)No sequence diagram generated since the changes primarily involve dependency updates, simplification of internal logic, test adjustments, and documentation additions without altering control flow. Warning Review ran into problems🔥 ProblemsGitHub checks timed out after 90 seconds. Some checks were still in progress when the timeout was reached. Consider increasing the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (3)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
cpp_linter_hooks/util.py (1)
149-158: Return type is inconsistent with surrounding helpers
_install_tool’s body returnsshutil.which(tool)(a str or None), whereas callers expect a Path.
Either cast withPath()here or update callers + type hints. Updating here keeps the public surface coherent:- return shutil.which(tool) + found = shutil.which(tool) + return Path(found) if found else None
🧹 Nitpick comments (5)
cpp_linter_hooks/util.py (5)
13-23: Docstring omits parameter / return detailsThe one-liner is helpful but doesn’t specify that the function returns an Optional[str] and what
Nonemeans. Consider expanding to a short Google/NumPy style block with Args / Returns so callers know the semantics.
111-133: Clarify “partial-version” behaviour in docstring
_resolve_versionsupports major‐only and major.minor inputs. Stating that explicitly will spare readers a deep dive into the code.
135-147: Mention clang-tidy special-case in docstringBecause the second line of
clang-tidy --versionis parsed, the docstring should indicate that it expects LLVM-style multi-line output and may return None if parsing fails.
161-173: Docstring could highlight side-effect (installation)Explicitly noting that the function may perform a network install via
pipwill help downstream consumers decide whether they can call this in restricted environments.
198-206: Docstring hides fallback behaviour
ensure_installedlogs a warning but still returns the tool name when installation fails. That subtlety is worth a sentence in the docstring so users don’t assume success.
shenxianpeng
changed the title
toml and packaging packges
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
cpp_linter_hooks/util.py (2)
138-146: Installation path builds an invalid requirement whenversion is NoneWhen
_resolve_install()falls back toNone, the current code calls:subprocess.check_call([... f"{tool}=={version}"])which becomes e.g.
clang-format==None– an invalid specifier that fails pip resolution.-def _install_tool(tool: str, version: str) -> Optional[Path]: +def _install_tool(tool: str, version: Optional[str]) -> Optional[Path]: @@ - subprocess.check_call( - [sys.executable, "-m", "pip", "install", f"{tool}=={version}"] - ) + requirement = f"{tool}=={version}" if version else tool + subprocess.check_call([sys.executable, "-m", "pip", "install", requirement])Also update the signature’s type hint in the call sites.
164-174: Version-mismatch logic fires whenuser_version is Noneif runtime_version and user_version not in runtime_version:If
user_versionisNone, the condition always evaluatesTrue, forcing an unnecessary reinstall (and crashing later as described above).- if runtime_version and user_version not in runtime_version: + if runtime_version and user_version and user_version not in runtime_version:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cpp_linter_hooks/util.py(5 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
cpp_linter_hooks/util.py
[refactor] 124-124: Either all return statements in a function should return an expression, or none of them should.
(R1710)
🪛 GitHub Actions: CodSpeed
cpp_linter_hooks/util.py
[error] 17-17: AttributeError: 'PosixPath' object has no attribute 'read' in function get_version_from_dependency when calling tomllib.load. The code incorrectly passes a PosixPath object instead of a file object.
🔇 Additional comments (2)
cpp_linter_hooks/util.py (1)
4-4: Import compatibility for < 3.11A back-port named
tomli, nottomllib, is what the majority of projects depend on.
Double-check that the PyPI packagetomllibyou pin really mirrors the std-lib API and won’t clash with 3.11+ environments wheretomllibis builtin.pyproject.toml (1)
35-38: Verify the back-port choiceYou now depend on
"tomllib; python_version < '3.11'",Unlike the widespread
tomli, thetomllibback-port is less battle-tested. Please ensure:
- It provides the same API surface as the std-lib module.
- It does not shadow/override the builtin on 3.11+.
Consider switching to
tomliplus the canonical shim:try: import tomllib # py3.11+ except ModuleNotFoundError: # <3.11 import tomli as tomllib
|
CodSpeed Performance ReportMerging #89 will degrade performances by 59.78%Comparing Summary
Benchmarks breakdown
|
shenxianpeng
changed the title
toml and packaging packgestoml and packaging and improve performance
Summary by CodeRabbit