unused_code: handle git grep no-matches and untracked#203
Conversation
…fixtures incl. param usage; fail CI on unexpected git errors; aggregate all unused functions per file; silence coverage show_contexts warning; fix jira_utils config precedence for tests
…h subprocess.CompletedProcess and avoid AttributeError in _git_grep
WalkthroughAdds git-grep probing, pytest-fixture-aware usage detection, portable regex builders, and single-file/ directory CLI options to the unused-code analyzer; expands tests, manifests, and README; adjusts file discovery helper; updates coverage config and .gitignore; whitespace-only change in Jira utils. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
![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 (6)
apps/jira_utils/jira_information.py (1)
153-160: Config-file precedence: consider falling back to CLI/env if keys are missing in the fileCurrent logic overwrites url/token with empty strings when a config file path is provided but the file lacks those keys, causing an immediate exit. If the intent is strict reproducibility, this is fine. If not, prefer config values when present, otherwise fall back to CLI/env to avoid surprising failures.
Proposed tweak:
- if config_file_path: - url = config_dict.get("url", "") - token = config_dict.get("token", "") + if config_file_path: + # Prefer config values when provided, otherwise fall back to CLI/env + url = config_dict.get("url") or url + token = config_dict.get("token") or tokenPlease confirm which behavior you want long-term (strict vs. fallback).
tests/unused_code/test_unused_code.py (2)
47-76: Good coverage for fixture-used-as-parameter; consider adding a test for “commented usage”This test validates the parameter-usage path for fixtures. To harden behavior, add a test ensuring a function name appearing only in a commented line isn’t treated as “used.” This will catch regressions around comment filtering.
I can draft a test that writes a file with a commented-out call and asserts the function is still reported unused.
117-126: Improve fidelity of the subprocess mock_git_grep uses text=True, so stdout/stderr are str in reality. Returning bytes works here but diverges from actual types. Prefer strings or use subprocess.CompletedProcess for realism.
class FakeCompleted: def __init__(self): self.returncode = 2 - self.stdout = b"" - self.stderr = b"fatal: not a git repository" + self.stdout = "" + self.stderr = "fatal: not a git repository"apps/unused_code/unused_code.py (3)
34-60: Pytest fixture detection: robust and readableDecorator handling covers both @pytest.fixture and from pytest import fixture forms, with and without parentheses. Looks good. Optional: support aliased imports (e.g., from pytest import fixture as fx) if needed in your codebase.
62-88: _git_grep: correct handling for rc=0/1 and unexpected errorsPCRE and untracked coverage are nice touches. Optional: add -I to ignore binary files for safety in mixed repos.
202-224: Exit code on processing failure: confirm desired semanticsExceptions during file processing exit with code 2. A prior team preference (retrieved learning) was to exit(1) immediately on file read/parse failures. Do we want to standardize on 1 for any failure, or keep 1 = “unused found” vs 2 = “processing error”?
If you prefer exit(1) for these failures, I can provide a patch; otherwise, please document the exit code semantics in the CLI help for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/jira_utils/jira_information.py(1 hunks)apps/unused_code/unused_code.py(4 hunks)pyproject.toml(1 hunks)tests/unused_code/test_unused_code.py(2 hunks)tests/unused_code/unused_code_file_for_test.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-12-29T12:05:41.200Z
Learnt from: myakove
PR: RedHatQE/python-utility-scripts#135
File: apps/unused_code/unused_code.py:61-62
Timestamp: 2024-12-29T12:05:41.200Z
Learning: When file reading or parsing fails in apps/unused_code/unused_code.py, we want to fail (use sys.exit(1)) immediately instead of continuing.
Applied to files:
tests/unused_code/test_unused_code.pytests/unused_code/unused_code_file_for_test.py
📚 Learning: 2024-12-29T11:59:22.364Z
Learnt from: myakove
PR: RedHatQE/python-utility-scripts#135
File: apps/unused_code/unused_code.py:0-0
Timestamp: 2024-12-29T11:59:22.364Z
Learning: When a function name retrieved from the AST is not found by 'git grep', the user wants a RuntimeError raised, instead of silently returning or skipping.
Applied to files:
apps/unused_code/unused_code.py
🧬 Code Graph Analysis (2)
tests/unused_code/test_unused_code.py (1)
apps/unused_code/unused_code.py (3)
_git_grep(62-87)get_unused_functions(192-227)process_file(112-171)
apps/unused_code/unused_code.py (1)
apps/utils.py (1)
all_python_files(76-86)
🔇 Additional comments (5)
tests/unused_code/unused_code_file_for_test.py (1)
1-7: LGTM: clearer stubs with explicit return typesSwitching to ellipsis stubs with None annotations is concise and keeps intent clear.
pyproject.toml (1)
48-48: Coverage HTML contexts disabledAcknowledged. Disabling show_contexts simplifies the report. If any tooling depends on contexts, ensure it’s updated.
tests/unused_code/test_unused_code.py (3)
1-3: LGTM: imports updated for new tests and helpersThe added imports align with the new fixture and grep behavior tests.
Also applies to: 6-6
78-96: LGTM: no-match handling verifiedSolid check that an empty grep result returns the “unused” message without crashing.
98-115: LGTM: autouse fixture skip verifiedAccurately ensures autouse fixtures are not flagged as unused.
…ate line content; ensure commented-line checks work reliably
…dout/stderr; fix formatting\nfix(unused_code): add -I to git grep to ignore binaries
…env for url/token to avoid unintended failures
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/unused_code/unused_code.py (2)
142-143:split(":", 2)still breaks on Windows pathsThe earlier review flagged that
git grepoutput should be parsed withrsplit(":", 2)to avoid mangling drive-letter paths likeC:/repo/file.py:12:.... Usingsplitfrom the left produces wrong_pathand_linesegments, so comment filtering and “own definition” checks fail again.- _path, _lineno, _line = entry.split(":", 2) + _path, _lineno, _line = entry.rsplit(":", 2)
160-163: Same parsing issue in fixture-parameter searchRepeat the
rsplit(":", 2)fix here to keep the logic consistent and correct.
🧹 Nitpick comments (2)
apps/unused_code/unused_code.py (1)
207-229: Thread-pool failure handling exits entire run
sys.exit(2)inside thefor future in as_completedloop aborts on the first file that raises, hiding subsequent errors. Collect all failures and print a summary instead so users can fix multiple issues in one run.tests/unused_code/test_unused_code.py (1)
65-71: Usepath.as_posix()to avoid OS-specific path separators in mockBuilding grep output with
f"{py_file}:1:..."relies on the OS path format. For cross-platform safety (especially Windows), convert thePathto POSIX first:py_file.as_posix().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/jira_utils/jira_information.py(1 hunks)apps/unused_code/unused_code.py(4 hunks)tests/unused_code/test_unused_code.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/jira_utils/jira_information.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-29T11:59:22.364Z
Learnt from: myakove
PR: RedHatQE/python-utility-scripts#135
File: apps/unused_code/unused_code.py:0-0
Timestamp: 2024-12-29T11:59:22.364Z
Learning: When a function name retrieved from the AST is not found by 'git grep', the user wants a RuntimeError raised, instead of silently returning or skipping.
Applied to files:
apps/unused_code/unused_code.py
🧬 Code Graph Analysis (1)
apps/unused_code/unused_code.py (1)
apps/utils.py (1)
all_python_files(76-86)
|
/verified |
…ce and cache; use detected flag for portability
…table patterns for -P/-G; keep comment filtering correct
…first; show summary\ntests(unused_code): use posix path in mock grep output and align with new call pattern
redhat-qe-bot
added
size/XL
and removed
verified
commented-coderabbitai[bot]
size/L
labels
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
apps/unused_code/unused_code.py (1)
203-208: Fix parsing of git-grep output; current split breaks on Windows drive letters and paths with colonsUse rsplit to reliably extract the last two fields (lineno and content). This also fixes comment filtering on the content.
- _path, _lineno, _line = entry.split(":", 2) + _path, _lineno, _line = entry.rsplit(":", 2) @@ - _path, _lineno, _line = entry.split(":", 2) + _path, _lineno, _line = entry.rsplit(":", 2)If you want, I can add a unit test asserting commented-out usages are ignored for both call-sites and fixture-parameter searches.
Also applies to: 221-228
🧹 Nitpick comments (1)
apps/unused_code/unused_code.py (1)
272-299: Small robustness improvements for concurrency and UX
- Pre-flight grep-flag detection before spawning threads to fail fast with a clear error if unsupported.
- Optionally stabilize output ordering for deterministic CI logs.
if not os.path.exists(".git"): LOGGER.error("Must be run from a git repository") sys.exit(1) + # Pre-flight: detect supported grep engine once and fail-fast if unsupported + try: + _detect_supported_grep_flag() + except RuntimeError as e: + LOGGER.error(str(e)) + sys.exit(2) + with ThreadPoolExecutor() as executor:Optional deterministic output (outside this hunk):
- Before printing, split lines and sort:
if unused_functions: lines = "\n".join(unused_functions).splitlines() click.echo("\n".join(sorted(lines))) sys.exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/unused_code/unused_code.py(6 hunks)tests/unused_code/test_unused_code.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unused_code/test_unused_code.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-29T11:59:22.364Z
Learnt from: myakove
PR: RedHatQE/python-utility-scripts#135
File: apps/unused_code/unused_code.py:0-0
Timestamp: 2024-12-29T11:59:22.364Z
Learning: When a function name retrieved from the AST is not found by 'git grep', the user wants a RuntimeError raised, instead of silently returning or skipping.
Applied to files:
apps/unused_code/unused_code.py
🧬 Code Graph Analysis (1)
apps/unused_code/unused_code.py (1)
apps/utils.py (1)
all_python_files(76-86)
🔇 Additional comments (2)
apps/unused_code/unused_code.py (2)
75-101: Fixture detection logic looks solidCovers both
@pytest.fixtureandfrom pytest import fixturewith/without parentheses. Reasonable tradeoff even without alias-resolution.
146-151: Behavior change: rc=1 returns [] (no matches)This aligns with the PR objective “handle git grep no-matches.” Note: a prior preference (from learnings) requested raising on unfound function names. Please confirm this new behavior supersedes the old expectation.
I’ll proceed assuming the new behavior is intended.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/unused_code/unused_code.py (1)
465-469: Safer split of git grep output to handle rare path colonsUse rsplit(":", 2) to split from the right; this avoids edge cases if a path ever contains a colon (less likely on Windows due to relative paths, but still safer).
- parts = entry.split(":", 2) + parts = entry.rsplit(":", 2) @@ - parts = entry.split(":", 2) + parts = entry.rsplit(":", 2)Also applies to: 498-505
🧹 Nitpick comments (7)
apps/utils.py (2)
76-89: Prune excluded dirs in-place and normalize directory path for consistent, faster walks
- Use in-place pruning of dirs to avoid descending into excluded trees; current substring check still traverses them and can be slow on large repos.
- Normalize provided directory to an absolute path for consistent outputs regardless of CLI input.
- Consider adding ".venv" to the excludes (fairly common).
Apply:
def all_python_files(directory: click.Path | None = None) -> Iterable[str]: """ Get all python files from current directory and subdirectories """ - exclude_dirs = [".tox", "venv", ".pytest_cache", "site-packages", ".git"] - target = str(directory) if directory else os.path.abspath(os.curdir) + exclude_dirs = {".tox", "venv", ".venv", ".pytest_cache", "site-packages", ".git"} + target = os.path.abspath(str(directory)) if directory else os.path.abspath(os.curdir) - for root, _, files in os.walk(target): - if [_dir for _dir in exclude_dirs if _dir in root]: - continue + for root, dirs, files in os.walk(target, topdown=True): + # Prevent walking into excluded directories + dirs[:] = [d for d in dirs if d not in exclude_dirs] for filename in files: if filename.endswith(".py"): yield os.path.join(root, filename)
76-83: Docstring should mention the optional directory argument and absolute yielding behaviorThe docstring still reflects “current directory” only. Please clarify that passing directory scopes the walk root. If you accept the abspath change above, note that yielded paths are absolute.
tests/unused_code/manifests/functions_as_args.py (1)
5-9: Use typing.Callable instead of builtin callable in annotationsThe builtin callable is a function, not a PEP 484 type. Use typing.Callable (optionally with ellipsis/return type). Future annotations won’t save you from static typing here.
-from __future__ import annotations +from __future__ import annotations +from typing import Any, Callable @@ -class TimeoutSampler: - def __init__(self, wait_timeout: int, sleep: int, func: callable, **func_kwargs) -> None: +class TimeoutSampler: + def __init__(self, wait_timeout: int, sleep: int, func: Callable[..., Any], **func_kwargs) -> None:tests/unused_code/test_unused_code_coverage.py (2)
138-149: Enable the documentation-pattern test once the parser bug is fixedOnce _is_documentation_pattern correctly matches indented “name (type): ...” lines (see my comment in apps/unused_code/unused_code.py), this xfail can be removed to assert the behavior.
-@pytest.mark.xfail @pytest.mark.parametrize( ("line", "is_doc"), [ (" my_function (str): description", True), ("if my_function(): pass", False), ("def my_function(): pass", False), (" # my_function()", True), ], ) def test_is_documentation_pattern(line, is_doc): assert _is_documentation_pattern(line, "my_function") == is_doc
152-159: Assert the precise exception type from _git_grep for clearer intent_git_grep wraps git errors in RuntimeError. Asserting Exception is broad and could mask regressions.
with pytest.raises(Exception): - _git_grep("pattern") + _git_grep("pattern")Follow-up:
- with pytest.raises(Exception): + with pytest.raises(RuntimeError): _git_grep("pattern")apps/unused_code/unused_code.py (2)
462-486: Reduce false positives: ignore string-literal-only occurrences in general usage scanWhole-word matching is intended to catch references, but it will also treat occurrences inside string literals as “usage” (e.g., logging). For non-fixture scanning, skip lines where the match is quoted.
# Ignore commented lines (full line or inline) code_part = _line.split("#", 1)[0] if func.name not in code_part: continue + # Ignore occurrences inside string literals like "my_function" or 'my_function' + if re.search(rf'(?<!\\)(["\'])\s*{re.escape(func.name)}\s*\1', code_part): + continue # If we are here, it's a valid usage. used = True breakNote: This does not affect fixture-specific string checks below.
573-580: Mutual exclusivity: make --file-path and --directory conflict explicitCurrently both options can be provided; code silently prefers file_path. Be explicit and fail fast to avoid user confusion.
if file_path and not os.path.isfile(str(file_path)): LOGGER.error("File path must be a file, not a directory.") sys.exit(1) if directory and not os.path.isdir(str(directory)): LOGGER.error("Directory must be a directory, not a file.") sys.exit(1) + + if file_path and directory: + LOGGER.error("Please pass only one of --file-path or --directory, not both.") + sys.exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
.gitignore(1 hunks)apps/unused_code/README.md(1 hunks)apps/unused_code/unused_code.py(7 hunks)apps/utils.py(1 hunks)tests/unused_code/manifests/functions_as_args.py(1 hunks)tests/unused_code/manifests/test_config.yaml(1 hunks)tests/unused_code/manifests/unused_code_file_for_test.py(1 hunks)tests/unused_code/test_unused_code.py(3 hunks)tests/unused_code/test_unused_code_coverage.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/unused_code/manifests/test_config.yaml
- apps/unused_code/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unused_code/test_unused_code.py
- .gitignore
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unused_code/test_unused_code_coverage.py (2)
apps/unused_code/unused_code.py (15)
_check_fixturenames_insert_pattern(149-179)_check_getfixturevalue_pattern(182-218)_find_git_root(345-362)_git_grep(365-404)_is_documentation_pattern(284-342)_is_pytest_mark_usefixtures_call(134-146)_is_usefixtures_context(221-281)_iter_functions(407-416)_resolve_absolute_path(429-434)get_unused_functions(563-635)is_fixture_autouse(59-70)is_ignore_function_list(419-426)is_pytest_fixture(73-98)process_file(437-530)_detect_supported_grep_flag(24-56)tests/utils.py (1)
get_cli_runner(4-5)
apps/unused_code/unused_code.py (1)
apps/utils.py (2)
get_util_config(14-18)all_python_files(76-88)
🔇 Additional comments (2)
tests/unused_code/manifests/unused_code_file_for_test.py (1)
1-219: Good coverage of documentation-like patterns and real usagesThis manifest thoughtfully exercises many doc patterns and legitimate calls. It should help stabilize false-positive filtering.
apps/unused_code/unused_code.py (1)
101-115: Confirm intent: usage pattern now matches any whole-word reference (not only call sites)The new _build_usage_pattern switches from call-site to whole-word matching to pick up references (e.g., passing functions as args). This increases recall but may increase false positives (strings, comments). With the string-literal filter above and existing doc filtering, this seems reasonable. Please confirm this trade-off is intended.
…o fix/unusedcode-handle-git-grep-no-matches-and-untracked
|
@dbasunag Please check again with the latest code |
|
/verified |
…o fix/unusedcode-handle-git-grep-no-matches-and-untracked
…o fix/unusedcode-handle-git-grep-no-matches-and-untracked
|
/verified |
|
/verified |
|
/approve |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/unused_code/unused_code.py (2)
462-469: Bug: incorrect split for git-grep output; breaks comment/def filtering and Windows pathsUse rsplit(":", 2) to safely extract line-content when paths contain colons (e.g., “C:/...”), and to drop the line number prefix from _line.
Apply:
- for entry in _git_grep(pattern=_build_usage_pattern(function_name=func.name), file_path=py_file): - # git grep -n output format: path:line-number:line-content - parts = entry.split(":", 2) - if len(parts) != 3: - continue - _, _, _line = parts + for entry in _git_grep(pattern=_build_usage_pattern(function_name=func.name), file_path=py_file): + # git grep -n => <path>:<lineno>:<content>; rsplit handles paths with colons + try: + _, __, _line = entry.rsplit(":", 2) + except ValueError: + continue
499-505: Same parsing bug in fixture search loopUse rsplit here too; otherwise _line may still include the line number, breaking comment filtering and validators.
Apply:
- for entry in _git_grep(pattern=pattern, file_path=py_file): - parts = entry.split(":", 2) - if len(parts) != 3: - continue - _path, _lineno, _line = parts + for entry in _git_grep(pattern=pattern, file_path=py_file): + try: + _path, _lineno, _line = entry.rsplit(":", 2) + except ValueError: + continue
🧹 Nitpick comments (5)
apps/unused_code/unused_code.py (5)
134-147: Handle pytest.mark aliasing in usefixtures detectionCurrently only detects pytest.mark.usefixtures; misses “from pytest import mark as m; m.usefixtures(...)” and “import pytest as p; p.mark.usefixtures(...)”.
Apply:
def _is_pytest_mark_usefixtures_call(call_node: ast.Call) -> bool: @@ - if isinstance(call_node.func, ast.Attribute): - # Handle pytest.mark.usefixtures - if ( - call_node.func.attr == "usefixtures" - and isinstance(call_node.func.value, ast.Attribute) - and call_node.func.value.attr == "mark" - and isinstance(call_node.func.value.value, ast.Name) - and call_node.func.value.value.id == "pytest" - ): - return True + if isinstance(call_node.func, ast.Attribute) and call_node.func.attr == "usefixtures": + base = call_node.func.value + # pytest.mark.usefixtures(...) + if isinstance(base, ast.Attribute) and base.attr == "mark" and isinstance(base.value, ast.Name): + if base.value.id in {"pytest"}: + return True + # mark.usefixtures(...) when "from pytest import mark" + if isinstance(base, ast.Name) and base.id in {"mark"}: + return True return FalseFor full robustness, consider discovering aliases from import statements and matching against that set.
221-282: Context detection for usefixtures is thoroughCovers assignments, lists, and decorators. Will benefit from the aliasing improvement above.
284-343: Doc filtering: add Sphinx-style “:param name:” and keep indentation-aware checkGood call using the original line for the indented pattern. Add a Sphinx-style check to avoid false positives.
Apply:
@@ - # Pattern 2: Lines that look like type annotations in docstrings + # Pattern 2: Sphinx-style param docs e.g. ":param name: ..." / ":type name: str" + if re.search(rf":param\s+{re.escape(function_name)}\s*:", stripped_line, flags=re.IGNORECASE): + return True + if re.search(rf":type\s+{re.escape(function_name)}\s*:", stripped_line, flags=re.IGNORECASE): + return True + + # Pattern 3: Lines that look like type annotations in docstrings @@ - # Pattern 3: Lines that contain common documentation keywords near the function name + # Pattern 4: Lines that contain common documentation keywords near the function name @@ - # Pattern 4: Check for common docstring patterns + # Pattern 5: Check for common docstring patterns
471-488: Tighten import/comment filtering after removing inline commentsUse lstrip() to catch indented import/from lines; current startswith() can miss them.
Apply:
- code_part = _line.split("#", 1)[0] - if code_part.startswith("import") or code_part.startswith("from"): + code_part = _line.split("#", 1)[0].lstrip() + if code_part.startswith("import ") or code_part.startswith("from "): continue
554-565: Make --file-path and --directory mutually exclusiveCurrently both can be supplied; clarify intent and fail fast.
Apply:
@click.option("--verbose", "-v", default=False, is_flag=True) @@ def get_unused_functions( @@ ) -> None: @@ - if file_path and not os.path.isfile(str(file_path)): + if file_path and directory: + LOGGER.error("Options --file-path and --directory are mutually exclusive.") + sys.exit(1) + if file_path and not os.path.isfile(str(file_path)): LOGGER.error("File path must be a file, not a directory.") sys.exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/unused_code/unused_code.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/unused_code/unused_code.py (1)
apps/utils.py (2)
get_util_config(14-18)all_python_files(76-88)
🔇 Additional comments (8)
apps/unused_code/unused_code.py (8)
23-56: Portable grep detection: solid implementationGood use of lru_cache, rc handling (0/1), and output discarding. This will avoid spurious failures and is thread-safe.
101-115: Usage regex builder looks goodWord-boundary handling for -P/-G is correct and sufficiently strict for general references.
149-180: AST pattern for fixturenames.insert: looks goodAccurately checks constant-string arguments. Safe fallbacks on parse errors.
182-219: AST pattern for getfixturevalue: looks goodCovers positional and keyword (“argname=”) usages.
345-363: Git root discovery: OKIterative parent walk with .git check is fine; fallback is acceptable given the top-level repository check.
365-405: git grep wrapper: OKDynamic flag, -e usage, cwd scoping, rc handling are correct. capture_output is acceptable here.
595-601: Pre-flight grep detection: good UXEarly failure with a clear message is great.
634-638: Sorted, aggregated output: confirm intended groupingYou’re sorting per-file blocks, not individual lines. If the intent is “aggregate all unused functions per file,” this is fine. If deterministic global ordering is desired, splitlines() and sort at message level.
Option if needed:
- sorted_output = sorted(unused_functions) - click.echo("\n".join(sorted_output)) + lines: list[str] = [] + for block in unused_functions: + lines.extend(block.splitlines()) + click.echo("\n".join(sorted(lines)))
rnetser
deleted the
fix/unusedcode-handle-git-grep-no-matches-and-untracked
branch
7 participants
detect pytest fixtures incl. param usage; fail CI on unexpected git errors; aggregate all unused functions per file; silence coverage show_contexts warning; fix jira_utils config precedence for tests
fix 186
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
Style