Handle function usage with unpacking dictionaries#264
Merged
Conversation
Contributor
WalkthroughAdds keyword unpacking function usage detection to the unused code analyzer. A new helper function generates portable regex patterns for matching syntax like Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
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 |
Collaborator
|
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
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. |
dbasunag
force-pushed
the
keyword_unpacking
branch
from
0ce0dbc to
03926f9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/unused_code/unused_code.py (1)
353-353: Removing"*"fromdoc_starterscould regress markdown-style bullet detection for the general usage check.The removal is needed to prevent
**func()lines from being falsely classified as documentation. However, it also means that markdown-style bullet lines like* func_name(arg)in docstrings are no longer caught as documentation by Pattern 4, potentially causing false positives in the existing general-usage detection path.If this is an acceptable trade-off, a brief comment explaining the rationale would help future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/unused_code/unused_code.py` at line 353, Update the doc_starters change by adding a brief inline comment near the doc_starters declaration explaining why "*" was removed (to avoid false positives like "**func()") and explicitly call out the trade-off that markdown-style bullets ("* item") will no longer be treated as documentation by Pattern 4; if you want to preserve markdown bullets instead, add a separate markdown-bullet check or a more specific regex for bolded-function false positives rather than reintroducing "*" into doc_starters.tests/polarion/test_polarion_verify_tc_requirements_coverage.py (1)
54-71: Consider lettingCliRunnerhandlesys.exitinstead of mocking it.Patching
sys.exitpreventsCliRunnerfrom capturing the actual exit code, making the assertionmock_exit.call_count >= 1more fragile than checkingresult.exit_code == 1. If code is later added aftersys.exit(1), it would execute unexpectedly under this mock. The same pattern appears at lines 279–296, 328–348, and 365–375.Without the
sys.exitpatch,CliRunnerwill catch theSystemExitand populateresult.exit_codedirectly:♻️ Suggested simplification (example for one test)
`@patch`("apps.polarion.polarion_verify_tc_requirements.get_polarion_project_id") `@patch`("apps.polarion.polarion_verify_tc_requirements.find_polarion_ids") `@patch`("apps.polarion.polarion_verify_tc_requirements.validate_polarion_requirements") - `@patch`("sys.exit") - def test_command_fails_with_missing_requirements(self, mock_exit, mock_validate, mock_find, mock_get_project): + def test_command_fails_with_missing_requirements(self, mock_validate, mock_find, mock_get_project): """Test command exits with code 1 when there are missing requirements""" # Arrange mock_get_project.return_value = "TEST_PROJECT" mock_find.return_value = ["TEST-001", "TEST-002"] mock_validate.return_value = ["TEST-001"] # Has missing requirements # Act - self.runner.invoke( + result = self.runner.invoke( has_verify, ["--project-id", "TEST_PROJECT"], ) # Assert - # sys.exit(1) should be called due to missing requirements - assert mock_exit.call_count >= 1 - assert 1 in [call.args[0] for call in mock_exit.call_args_list] + assert result.exit_code == 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/polarion/test_polarion_verify_tc_requirements_coverage.py` around lines 54 - 71, Remove the patching of sys.exit in test_command_fails_with_missing_requirements and let CliRunner handle SystemExit: call result = self.runner.invoke(has_verify, ["--project-id", "TEST_PROJECT"]) and assert result.exit_code == 1 instead of inspecting mock_exit; also remove mock_exit from the test signature and any assertions on mock_exit.call_args_list. Apply the same change to the other tests that patch sys.exit (the ones around the later blocks) so they all use result = self.runner.invoke(...) and assert result.exit_code for exit behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/unused_code/unused_code.py`:
- Around line 506-528: The keyword-unpacking usage check wrongly treats lines
with inline comments like "x = 1 # **func_name()" as real usage because it only
tests _line.strip().startswith("#"); update the block that iterates _git_grep
(the loop using _build_keyword_unpacking_pattern, _is_documentation_pattern and
variable _line) to reuse the same inline-comment stripping logic as the general
check: split _line on "#" once (e.g., _line.split("#", 1)[0]), assign the code
portion, skip if empty or if _is_documentation_pattern still matches, and then
test for the keyword-unpacking pattern/function_name in the code portion before
marking used = True.
In `@tests/unused_code/test_unused_code_keyword_unpacking.py`:
- Around line 126-160: The test test_keyword_unpacking_with_cli creates a temp
file outside the repo so git grep won't find usages; update the test to mock the
_git_grep call (the same way other tests do) so
process_file/get_unused_functions sees the temp file's symbol usages (or
alternatively write the temp file into the repo working tree); specifically
patch/mock the module function _git_grep used by process_file (and ensure
get_cli_runner()/get_unused_functions still invoke the patched behavior) so
get_config and get_defaults are not reported as unused.
---
Nitpick comments:
In `@apps/unused_code/unused_code.py`:
- Line 353: Update the doc_starters change by adding a brief inline comment near
the doc_starters declaration explaining why "*" was removed (to avoid false
positives like "**func()") and explicitly call out the trade-off that
markdown-style bullets ("* item") will no longer be treated as documentation by
Pattern 4; if you want to preserve markdown bullets instead, add a separate
markdown-bullet check or a more specific regex for bolded-function false
positives rather than reintroducing "*" into doc_starters.
In `@tests/polarion/test_polarion_verify_tc_requirements_coverage.py`:
- Around line 54-71: Remove the patching of sys.exit in
test_command_fails_with_missing_requirements and let CliRunner handle
SystemExit: call result = self.runner.invoke(has_verify, ["--project-id",
"TEST_PROJECT"]) and assert result.exit_code == 1 instead of inspecting
mock_exit; also remove mock_exit from the test signature and any assertions on
mock_exit.call_args_list. Apply the same change to the other tests that patch
sys.exit (the ones around the later blocks) so they all use result =
self.runner.invoke(...) and assert result.exit_code for exit behavior.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unused_code/test_unused_code_keyword_unpacking.py (1)
25-25: Consider using an explicitfile_pathparameter instead of**kwargsin mock functions.The
**kwargssilently absorbs thefile_pathkeyword argument. Using an explicit parameter improves readability and silences the Ruff ARG001 warning.Example for one mock (apply similarly to others)
- def _mock_grep(pattern: str, **kwargs): + def _mock_grep(pattern: str, file_path=None):Also applies to: 52-52, 78-78, 108-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unused_code/test_unused_code_keyword_unpacking.py` at line 25, The mock function _mock_grep currently uses **kwargs which swallows the file_path kwarg and triggers Ruff ARG001; change its signature to accept file_path explicitly (e.g., def _mock_grep(pattern: str, file_path: str):) and update its body to use that parameter, and apply the same change to the other mock functions in this file that currently use **kwargs (the mocks at the other comment locations) so the tests read file_path directly and the ARG001 warning is silenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/unused_code/unused_code.py`:
- Around line 522-524: The check that skips commented lines only tests
_line.strip().startswith("#"), which misses inline comments and allows lines
like "x = 1 # func.name()" to be treated as real usage; update the logic (used
alongside the general usage check that already does _line.split("#", 1)[0]) to
strip inline comments before deciding to continue — i.e., derive the code
portion via _line.split("#", 1)[0].strip() and if that code portion is empty
continue, and use that same code portion when checking for func.name to ensure
inline comments are not counted as real usage.
In `@tests/unused_code/test_unused_code_keyword_unpacking.py`:
- Around line 122-155: The CLI test test_keyword_unpacking_with_cli creates a
temp file outside the repo so the real _git_grep (which falls back to
os.getcwd()) scans the repository and makes the assertions flaky; update the
test to mock _git_grep (as in the other four tests) to return expected grep
results for get_config/get_defaults (or alternatively create the temp file
inside the repository working tree) so
get_cli_runner().invoke(get_unused_functions, ...) sees deterministic results
and the assertions about result.output become reliable.
---
Nitpick comments:
In `@tests/unused_code/test_unused_code_keyword_unpacking.py`:
- Line 25: The mock function _mock_grep currently uses **kwargs which swallows
the file_path kwarg and triggers Ruff ARG001; change its signature to accept
file_path explicitly (e.g., def _mock_grep(pattern: str, file_path: str):) and
update its body to use that parameter, and apply the same change to the other
mock functions in this file that currently use **kwargs (the mocks at the other
comment locations) so the tests read file_path directly and the ARG001 warning
is silenced.
Collaborator
|
/lgtm |
Collaborator
Author
|
/verified |
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
6 participants
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.
Summary by CodeRabbit
New Features
Tests