Skip unused code for functions#164
Conversation
WalkthroughThis pull request updates the documentation, the core processing logic, and the test suite for the unused code detection tool. The documentation now includes additional sections to clarify usage and configuration, including a method to skip individual functions via inline comments. In the core module, the AST parsing method has been updated and a condition added to bypass functions marked with a skip comment. The tests have been expanded to verify the new skip functionality. No changes were made to any exported or public entities. Changes
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
Report bugs in Issues The following are automatically added:
Available user actions:
Supported /retest check runs
Supported labels
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/unused_code/README.md(3 hunks)apps/unused_code/unused_code.py(3 hunks)tests/unused_code/test_unused_code.py(1 hunks)tests/unused_code/unused_code_file_for_test.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tox
- GitHub Check: python-module-install
🔇 Additional comments (6)
tests/unused_code/unused_code_file_for_test.py (1)
9-10: Implementation aligns with PR objectivesThe addition of
skip_with_comment()function with the# skip-unused-codecomment serves as a perfect test case for the new feature being introduced in this PR. This allows developers to mark certain functions to be skipped during unused code detection.apps/unused_code/README.md (2)
40-48: Clear documentation of the new featureThe documentation clearly explains how to use the new
# skip-unused-codecomment feature with a good example. This will help users understand and adopt the feature.
17-25: Improved YAML formatting enhances readabilityThe indentation changes in the YAML example improve consistency and readability of the configuration example.
apps/unused_code/unused_code.py (3)
62-62: AST parsing method updated to support commentsThe change from
ast.parsetoparsefrom theast_commentsmodule is appropriate for the feature, as it enables access to comments which are ignored by the standard AST parser.
78-78: Improved regex for function call detectionChanging from
-wto-wEin the git grep command improves the pattern matching by treating the pattern as an extended regular expression. This allows for more precise detection of function calls with arguments.
73-75:Details
❓ Verification inconclusive
New feature implementation to skip functions
This code correctly implements the feature to skip functions marked with the
# skip-unused-codecomment. The logic checks if any item in the function body has this specific comment value.However, it's worth noting that the implementation assumes the comment will be on a separate line within the function body, rather than inline with the function definition as shown in the README example.
Let's verify if inline comments (as shown in README) will work with this implementation:
🏁 Script executed:
#!/bin/bash # Create a test file with an inline comment cat << 'EOF' > /tmp/test_inline_comment.py def my_function(): # skip-unused-code pass EOF # Check how ast_comments parses this file python3 -c "from ast_comments import parse; import ast; tree = parse(open('/tmp/test_inline_comment.py').read()); print([item.value for item in tree.body[0].body if hasattr(item, 'value')])"Length of output: 336
Inline Comment Handling Requires Further Verification
The new feature correctly skips functions when the
# skip-unused-codecomment is placed on a separate line within the function body. However, the test for inline comments (e.g.,def my_function(): # skip-unused-code) resulted in aModuleNotFoundErrorfor theast_commentsmodule, leaving its behavior unverified. Please manually verify inline comment handling (or update the test environment to includeast_comments) to confirm it meets the README example expectations.
- File:
apps/unused_code/unused_code.py(Lines 73–75)- Issue: Inline comment parsing is unverified due to missing module in the test environment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unused_code/test_unused_code.py (1)
37-41: Consider clarifying the purpose of this testThis test appears very similar to
test_unused_code_function_list_exclude_all()on lines 23-27. Both tests:
- Call the same command with identical arguments
- Assert the same expected results
While I understand this test is likely validating the new skip-with-comment functionality, the test name and assertions don't make this explicit. Consider either:
- Renaming the test to clarify its purpose (e.g.,
test_skip_with_comment_functionality)- Adding a comment explaining how this test differs from the existing one
- Adding more specific assertions to validate the comment-based skipping
-def test_unused_code_check_skip_with_comment(): +def test_skip_with_comment_functionality(): result = get_cli_runner().invoke(get_unused_functions, '--exclude-function-prefixes "unused_code_"') LOGGER.info(f"Result output: {result.output}, exit code: {result.exit_code}, exceptions: {result.exception}") assert result.exit_code == 0 - assert "Is not used anywhere in the code" not in result.output + # Verify that skip_with_comment function isn't flagged as unused + assert "skip_with_comment" not in result.output + assert "Is not used anywhere in the code" not in result.outputapps/unused_code/unused_code.py (1)
73-75: Consider making the comment detection more flexibleThe current implementation looks for an exact match of
# skip-unused-code. You might want to consider making this more flexible by:
- Allowing different spacing (e.g.,
#skip-unused-codeor# skip-unused-code)- Making it case-insensitive
- Supporting placing the comment on a different line (e.g., as a docstring)
- if any(getattr(item, "value", None) == "# skip-unused-code" for item in func.body): + skip_pattern = "skip-unused-code" + if any(getattr(item, "value", None) and skip_pattern in getattr(item, "value", "").lower().strip() for item in func.body):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/unused_code/README.md(3 hunks)apps/unused_code/unused_code.py(3 hunks)tests/unused_code/test_unused_code.py(1 hunks)tests/unused_code/unused_code_file_for_test.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tox
- GitHub Check: python-module-install
🔇 Additional comments (7)
tests/unused_code/unused_code_file_for_test.py (1)
9-10: LGTM: Good test function for the new skip featureThe addition of this function with the
# skip-unused-codecomment provides a good test case for verifying the new feature's functionality.apps/unused_code/README.md (1)
41-48: LGTM: Clear documentation for the new featureThe documentation clearly explains how to use the new skip comment functionality with a good example.
apps/unused_code/unused_code.py (5)
12-12: LGTM: Added necessary import for comment parsingThe addition of the
ast_commentsimport is essential for the new feature that needs to parse code comments.
62-62: LGTM: Updated parsing to support commentsSwitching from standard AST parsing to the
ast_comments.parsefunction enables handling of comments in the code, which is necessary for the new skip functionality.
73-75: LGTM: Well-implemented comment detectionThe implementation efficiently checks for the presence of the skip comment in the function body using a generator expression.
78-78: Enhanced git grep pattern for better function detectionThe updated git grep command with
-wEflags and function argument pattern improves function usage detection:
-wensures whole word matching (prevents partial matches)-Eenables extended regex for the function call pattern- The pattern
{func.name}(.*)matches function calls with arguments
77-78:Details
✅ Verification successful
Verify that the new grep pattern doesn't miss any legitimate function calls
The change from the previous grep command to using
-wEwith a pattern that includes function arguments is a significant change that could affect how function usages are detected.
🏁 Script executed:
#!/bin/bash # Compare the results of the old and new grep patterns to ensure we don't miss any legitimate function calls # First, find a function name we can test with FUNC_NAME=$(grep -r "def " --include="*.py" . | head -1 | sed 's/.*def \([a-zA-Z0-9_]*\).*/\1/') echo "Testing with function: $FUNC_NAME" # Compare old style (without -wE and function args pattern) vs new style echo "Old style grep:" git grep "$FUNC_NAME" | grep -v "def $FUNC_NAME" echo "New style grep:" git grep -wE "$FUNC_NAME(.*)" | grep -v "def $FUNC_NAME" # Check if any results differ DIFF_COUNT=$(diff <(git grep "$FUNC_NAME" | grep -v "def $FUNC_NAME") <(git grep -wE "$FUNC_NAME(.*)" | grep -v "def $FUNC_NAME") | wc -l) echo "Number of differences: $DIFF_COUNT" if [ $DIFF_COUNT -gt 0 ]; then echo "Warning: The new grep pattern may miss some function calls" diff <(git grep "$FUNC_NAME" | grep -v "def $FUNC_NAME") <(git grep -wE "$FUNC_NAME(.*)" | grep -v "def $FUNC_NAME") fiLength of output: 1406
Grep Pattern Verification Confirmed: No Missed Function Calls Detected
The verification script shows that the updated grep pattern (-wEwith the function arguments pattern) produces the same results as the old grep command. In the test run using the functionget_issue, there were no differences identified (diff count of 0), confirming that all legitimate function calls are being correctly detected.
- Location:
apps/unused_code/unused_code.py, Lines 77-78Great work on this change—no further adjustments are required here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unused_code/test_unused_code.py (1)
37-41: Test might not be specifically testing the skip comment featureThis test appears to be duplicating the functionality of
test_unused_code_function_list_exclude_all()(lines 23-27). Both tests use the same--exclude-function-prefixes "unused_code_"parameter and have identical assertions. Consider modifying this test to specifically verify the comment-based skipping feature rather than function prefix exclusion.-def test_unused_code_check_skip_with_comment(): - result = get_cli_runner().invoke(get_unused_functions, '--exclude-function-prefixes "unused_code_"') +def test_unused_code_check_skip_with_comment(): + # Test that functions with '# skip-unused-code' comment are skipped without using exclusion prefixes + result = get_cli_runner().invoke(get_unused_functions) LOGGER.info(f"Result output: {result.output}, exit code: {result.exit_code}, exceptions: {result.exception}") assert result.exit_code == 0 assert "Is not used anywhere in the code" not in result.output
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/unused_code/README.md(3 hunks)apps/unused_code/unused_code.py(3 hunks)tests/unused_code/test_unused_code.py(1 hunks)tests/unused_code/unused_code_file_for_test.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tox
- GitHub Check: python-module-install
🔇 Additional comments (6)
tests/unused_code/unused_code_file_for_test.py (1)
9-10: Great implementation of the skippable function with comment!The function
skip_with_comment()correctly demonstrates how to use the new# skip-unused-codecomment to mark a function that should be excluded from unused code checks.apps/unused_code/README.md (1)
41-48: Clear documentation for the new skip feature!The documentation for skipping a single function using the
# skip-unused-codecomment is clear and includes a helpful example. This will make it easy for users to understand how to use this new feature.apps/unused_code/unused_code.py (4)
12-12: Good addition of the ast_comments parserUsing the
ast_commentsparser is essential for the new skip comment feature as it preserves comments that would be otherwise lost with the standard AST parser.
62-62: Correct implementation of the ast_comments parserUsing
parsefrom theast_commentsmodule instead ofast.parseenables the feature to detect and process comments in the code.
78-78: Improved function usage detectionUsing the
-wEflags with the enhanced regex pattern{func.name}(.*)will better match function calls with different parameter patterns, reducing false negatives in the detection.
73-75:Details
✅ Verification successful
Verify comment detection implementation
The implementation checks for the
# skip-unused-codecomment in the function body, but the example in the README shows the comment on the function definition line. This might lead to inconsistent behavior.Let's verify how the
ast_commentsparser represents comments that appear at the end of a function definition line:
🏁 Script executed:
#!/bin/bash # Create a test file with different comment placements cat > test_comments.py << 'EOF' def function_with_comment_on_def_line(): # skip-unused-code pass def function_with_comment_in_body(): # skip-unused-code pass EOF # Install ast-comments if not already installed pip install ast-comments > /dev/null 2>&1 # Create a script to parse and inspect the AST cat > inspect_ast.py << 'EOF' from ast_comments import parse with open('test_comments.py', 'r') as file: tree = parse(source=file.read()) for node in tree.body: if hasattr(node, 'name'): print(f"Function: {node.name}") print(f" Body items: {len(node.body)}") for item in node.body: if hasattr(item, 'value'): print(f" - Item value: {repr(item.value)}") EOF # Run the inspection script python inspect_ast.pyLength of output: 252
Consistent Comment Detection Verified
The test results confirm that regardless of whether the
# skip-unused-codecomment is placed on the function definition line or within the function body, theast-commentsparser represents it as a body item with the same value. This shows that the current implementation—which checks for the comment in the function’s body—is consistent with the behavior described in the README.No code changes are required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unused_code/test_unused_code.py (1)
37-41: Consider whether this duplicate test is necessary.This test appears to be functionally identical to the existing
test_unused_code_function_list_exclude_all()on lines 23-27. Both tests:
- Run the same command with the same parameters (
--exclude-function-prefixes "unused_code_")- Assert the same conditions (exit code 0 and absence of "not used" message)
If the purpose is to specifically test the new skip comment functionality, consider either:
- Removing the duplicate test and modifying the test case name to reflect both exclusion methods
- Making this test more focused on the skip comment feature by using different parameters
-def test_unused_code_check_skip_with_comment(): - result = get_cli_runner().invoke(get_unused_functions, '--exclude-function-prefixes "unused_code_"') +def test_unused_code_check_skip_with_comment(): + # Test specifically for the skip-unused-code comment functionality + result = get_cli_runner().invoke(get_unused_functions) LOGGER.info(f"Result output: {result.output}, exit code: {result.exit_code}, exceptions: {result.exception}") assert result.exit_code == 0 assert "Is not used anywhere in the code" not in result.output
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/unused_code/README.md(3 hunks)apps/unused_code/unused_code.py(3 hunks)tests/unused_code/test_unused_code.py(1 hunks)tests/unused_code/unused_code_file_for_test.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tox
- GitHub Check: python-module-install
🔇 Additional comments (7)
tests/unused_code/unused_code_file_for_test.py (1)
7-10: LGTM! Example function added for testing the skip comment feature.The new
skip_with_comment()function correctly implements the skip-unused-code comment pattern and will serve as a good test case for verifying that functions with this comment are properly excluded from unused code checks.apps/unused_code/README.md (1)
40-48: LGTM! Clear documentation for the new feature.The added section clearly explains how to skip a single function in a file by adding the
# skip-unused-codecomment. The example is concise and demonstrates the correct syntax. Good documentation practice!apps/unused_code/unused_code.py (5)
12-12: LGTM! Added required dependency for comment parsing.The import of
parsefrom theast_commentsmodule is necessary to handle the new comment-based functionality.
62-62: LGTM! Replaced standard AST parser with comment-aware parser.Using
parsefromast_commentsinstead ofast.parseis necessary as the standard AST parser doesn't preserve comments. This change enables the skip-unused-code feature to work.
73-76: LGTM! Core implementation of the skip-unused-code feature.The added condition correctly checks for the presence of the "# skip-unused-code" comment in the function body and skips processing if found. The debug log message provides useful information for troubleshooting.
78-78: Improved function usage detection with better regex pattern.The grep command has been improved to use word boundaries (
-w) and extended regex (-E), which will provide more accurate function usage detection and reduce false positives.
73-78:Details
❓ Verification inconclusive
Verify if comments in function body are properly detected.
The implementation checks for the comment in
func.body, but the documentation and example suggest placing the comment at the function definition line. Make sure the AST parser correctly captures comments placed at the function signature line.
🏁 Script executed:
#!/bin/bash # Create a temporary test file with different comment placements cat > test_comment_detection.py << 'EOL' def function_with_comment_after_def(): # skip-unused-code pass def function_with_comment_in_body(): # skip-unused-code pass EOL # Use the ast_comments parser to inspect the AST python3 -c " from ast_comments import parse with open('test_comment_detection.py', 'r') as f: tree = parse(source=f.read()) print('== Function 1 ==') for item in tree.body[0].body: print(f'Body item value: {getattr(item, \"value\", None)}') print('\\n== Checking Function 1 with condition ==') print(f'Would be skipped: {any(getattr(item, \"value\", None) == \"# skip-unused-code\" for item in tree.body[0].body)}') print('\\n== Function 2 ==') for item in tree.body[1].body: print(f'Body item value: {getattr(item, \"value\", None)}') print('\\n== Checking Function 2 with condition ==') print(f'Would be skipped: {any(getattr(item, \"value\", None) == \"# skip-unused-code\" for item in tree.body[1].body)}') " # Clean up rm test_comment_detection.pyLength of output: 938
Action Required: Verify AST Comment Detection on Function Signatures
The automated test using the
ast_commentsmodule failed with aModuleNotFoundError. This prevents us from confirming that comments placed on the function definition line (inline comments) are properly captured. As the current logic only inspectsfunc.body, there’s still uncertainty whether inline comments (e.g.,def foo(): # skip-unused-code) would be detected correctly per the documentation.• Please manually verify that the AST parser (or the alternative method used for comment detection) correctly identifies the
# skip-unused-codecomment when it’s placed on the function signature line.
• If needed, adjust the logic to inspect both the function signature and its body.
|
/verified |
7 participants
Support for skipping function using comment in the code.
For example:
Summary by CodeRabbit