Add ability to skip a single function def, fix relative paths for exclude-files in unused-code#156
Add ability to skip a single function def, fix relative paths for exclude-files in unused-code#156lugi0 wants to merge 4 commits into
Conversation
Signed-off-by: lugi0 <lgiorgi@redhat.com>
WalkthroughThe changes introduce a new function, Changes
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 3
🧹 Nitpick comments (2)
apps/unused_code/unused_code.py (2)
111-112: File is read twice, which may impact performance.Reading the file twice (once for AST parsing and once for comment extraction) may impact performance on large files or codebases. Consider reading the file once and reusing the content.
with open(py_file) as fd: - tree = ast.parse(source=fd.read()) + source_code = fd.read() + tree = ast.parse(source=source_code) -with open(py_file) as fd: - comments = extract_function_comments(source_code=fd.read()) +comments = extract_function_comments(source_code=source_code)
149-165: Consider adding documentation about new skip comment feature in CLI help.The CLI help text for existing options is detailed, but there's no mention of the new skip comment feature. Adding information about this feature would improve discoverability.
Consider adding a note about the new skip comment feature to the CLI help text, e.g.:
@click.option( "--exclude-function-prefixes", help="Provide a comma-separated string or list of function prefixes to exclude", type=ListParamType(), ) +@click.help_option('--help', '-h', help="Show this message and exit. Note: You can skip individual functions by adding '# skip-unused-code' comment before the function definition.") @click.option("--verbose", "-v", default=False, is_flag=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/unused_code/unused_code.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tox
- GitHub Check: python-module-install
🔇 Additional comments (4)
apps/unused_code/unused_code.py (4)
8-10: Added necessary imports for tokenization.The new imports are correctly added to support the new function that extracts comments from source code.
19-19: Good constant definition.Defining a constant for the skip comment makes the code more maintainable and prevents magic strings throughout the codebase.
104-104: Fixed file exclusion to use relative paths.The change from using basename to relative path is correct and aligns with the PR objective to fix relative paths for exclude-files. This allows for more targeted exclusion of files.
22-26: Clear and effective docstring.The docstring is clear and effectively explains the purpose of the function, along with an example of how to use the skip comment.
| if SKIP_COMMENT in comments[func.name]: | ||
| LOGGER.debug(f"Skipping function: {func.name} due to comment") | ||
| continue |
There was a problem hiding this comment.
Need to handle potential KeyError in comments dictionary.
The code doesn't check if the function name exists in the comments dictionary before accessing it, which could raise a KeyError if a function doesn't have any associated comments.
for func in _iter_functions(tree=tree):
- if SKIP_COMMENT in comments[func.name]:
+ if func.name in comments and SKIP_COMMENT in comments[func.name]:
LOGGER.debug(f"Skipping function: {func.name} due to comment")
continue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if SKIP_COMMENT in comments[func.name]: | |
| LOGGER.debug(f"Skipping function: {func.name} due to comment") | |
| continue | |
| for func in _iter_functions(tree=tree): | |
| if func.name in comments and SKIP_COMMENT in comments[func.name]: | |
| LOGGER.debug(f"Skipping function: {func.name} due to comment") | |
| continue |
| def extract_function_comments(source_code: str) -> dict[str, list[str]]: | ||
| """ | ||
| Finds the comments preceding function definitions, which can be used to skip single functions with a specific comment, | ||
| i.e. `# skip-unused-code` | ||
| """ | ||
| # Create an AST from the source code | ||
| tree = ast.parse(source_code) | ||
|
|
||
| # Initialize a dictionary to hold comments for each function | ||
| function_comments: dict[str, list[str]] = {} | ||
|
|
||
| # Tokenize the source code to find comments | ||
| tokens = tokenize.generate_tokens(StringIO(source_code).readline) | ||
|
|
||
| # To store the comments for each function | ||
| function_comments = {} | ||
| current_function = None | ||
| comments: list[str] = [] | ||
| function_start_line = None | ||
|
|
||
| # Process the tokens and extract comments | ||
| for token in tokens: | ||
| if token.type == tokenize.COMMENT: | ||
| # Associate the comment with the current function, if any | ||
| if current_function is not None: | ||
| comments.append(token.string) | ||
|
|
||
| # Detect the start of a new function definition | ||
| elif token.type == tokenize.NAME and token.string == "def": | ||
| # Record the function's starting line | ||
| function_start_line = token.start[0] | ||
|
|
||
| elif token.type == tokenize.NAME and token.string != "def": | ||
| if function_start_line is not None: | ||
| # Find the function name in the AST node | ||
| for node in ast.walk(tree): | ||
| if isinstance(node, ast.FunctionDef) and node.lineno == function_start_line: | ||
| current_function = node.name | ||
| function_comments[current_function] = comments | ||
| function_start_line = None | ||
| comments = [] | ||
|
|
||
| return function_comments |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function comment extraction implementation needs improvement.
The function has a few issues:
function_commentsis defined twice (lines 31 and 37)- The function doesn't handle edge cases well - like when a function name doesn't exist in comments
- There's no error handling if parsing fails
Here's a suggested improvement:
def extract_function_comments(source_code: str) -> dict[str, list[str]]:
"""
Finds the comments preceding function definitions, which can be used to skip single functions with a specific comment,
i.e. `# skip-unused-code`
"""
# Create an AST from the source code
tree = ast.parse(source_code)
# Initialize a dictionary to hold comments for each function
- function_comments: dict[str, list[str]] = {}
+ function_comments: dict[str, list[str]] = {}
# Tokenize the source code to find comments
tokens = tokenize.generate_tokens(StringIO(source_code).readline)
# To store the comments for each function
- function_comments = {}
current_function = None
comments: list[str] = []
function_start_line = None
# Process the tokens and extract comments
for token in tokens:
if token.type == tokenize.COMMENT:
# Associate the comment with the current function, if any
if current_function is not None:
comments.append(token.string)
# Detect the start of a new function definition
elif token.type == tokenize.NAME and token.string == "def":
# Record the function's starting line
function_start_line = token.start[0]
elif token.type == tokenize.NAME and token.string != "def":
if function_start_line is not None:
# Find the function name in the AST node
for node in ast.walk(tree):
if isinstance(node, ast.FunctionDef) and node.lineno == function_start_line:
current_function = node.name
function_comments[current_function] = comments
function_start_line = None
comments = []
return function_comments📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def extract_function_comments(source_code: str) -> dict[str, list[str]]: | |
| """ | |
| Finds the comments preceding function definitions, which can be used to skip single functions with a specific comment, | |
| i.e. `# skip-unused-code` | |
| """ | |
| # Create an AST from the source code | |
| tree = ast.parse(source_code) | |
| # Initialize a dictionary to hold comments for each function | |
| function_comments: dict[str, list[str]] = {} | |
| # Tokenize the source code to find comments | |
| tokens = tokenize.generate_tokens(StringIO(source_code).readline) | |
| # To store the comments for each function | |
| function_comments = {} | |
| current_function = None | |
| comments: list[str] = [] | |
| function_start_line = None | |
| # Process the tokens and extract comments | |
| for token in tokens: | |
| if token.type == tokenize.COMMENT: | |
| # Associate the comment with the current function, if any | |
| if current_function is not None: | |
| comments.append(token.string) | |
| # Detect the start of a new function definition | |
| elif token.type == tokenize.NAME and token.string == "def": | |
| # Record the function's starting line | |
| function_start_line = token.start[0] | |
| elif token.type == tokenize.NAME and token.string != "def": | |
| if function_start_line is not None: | |
| # Find the function name in the AST node | |
| for node in ast.walk(tree): | |
| if isinstance(node, ast.FunctionDef) and node.lineno == function_start_line: | |
| current_function = node.name | |
| function_comments[current_function] = comments | |
| function_start_line = None | |
| comments = [] | |
| return function_comments | |
| def extract_function_comments(source_code: str) -> dict[str, list[str]]: | |
| """ | |
| Finds the comments preceding function definitions, which can be used to skip single functions with a specific comment, | |
| i.e. `# skip-unused-code` | |
| """ | |
| # Create an AST from the source code | |
| tree = ast.parse(source_code) | |
| # Initialize a dictionary to hold comments for each function | |
| function_comments: dict[str, list[str]] = {} | |
| # Tokenize the source code to find comments | |
| tokens = tokenize.generate_tokens(StringIO(source_code).readline) | |
| # To store the comments for each function | |
| current_function = None | |
| comments: list[str] = [] | |
| function_start_line = None | |
| # Process the tokens and extract comments | |
| for token in tokens: | |
| if token.type == tokenize.COMMENT: | |
| # Associate the comment with the current function, if any | |
| if current_function is not None: | |
| comments.append(token.string) | |
| # Detect the start of a new function definition | |
| elif token.type == tokenize.NAME and token.string == "def": | |
| # Record the function's starting line | |
| function_start_line = token.start[0] | |
| elif token.type == tokenize.NAME and token.string != "def": | |
| if function_start_line is not None: | |
| # Find the function name in the AST node | |
| for node in ast.walk(tree): | |
| if isinstance(node, ast.FunctionDef) and node.lineno == function_start_line: | |
| current_function = node.name | |
| function_comments[current_function] = comments | |
| function_start_line = None | |
| comments = [] | |
| return function_comments |
Signed-off-by: lugi0 <lgiorgi@redhat.com>
|
@myakove @dbasunag addressed the comments. I've updated the logic to only look for inline comments on function definitions equal to I've also slightly tweaked the logic of I've also updated and added test cases to check the updated logic all around. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/unused_code/unused_code.py (1)
124-126:⚠️ Potential issueFix potential KeyError when checking comments.
The code doesn't check if the function name exists in the comments dictionary before accessing it, which could raise a KeyError if a function doesn't have any associated comments.
- if func.name in comments.keys(): + if func.name in comments:
🧹 Nitpick comments (7)
apps/unused_code/README.md (1)
37-40: Add a comma for better readability.There should be a comma after "repository" to improve readability.
-To skip single functions in your target repository you can add an inline comment to the function definition. The comment should match `# skip-unused-code` +To skip single functions in your target repository, you can add an inline comment to the function definition. The comment should match `# skip-unused-code`🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: Possible missing comma found.
Context: ...To skip single functions in your target repository you can add an inline comment to the fu...(AI_HYDRA_LEO_MISSING_COMMA)
apps/unused_code/unused_code.py (4)
22-26: Improve docstring clarity.The docstring could be more specific about what this function returns and how it's used.
- """ - Finds *only* inline comments for function definition that match `SKIP_COMMENT` and returns them - """ + """ + Finds *only* inline comments for function definitions that match `SKIP_COMMENT`. + + Returns: + dict[str, list[str]]: A dictionary mapping function names to a list of their inline comments + that match the SKIP_COMMENT pattern. + """
34-71: Handle potential errors in comment extraction.The function doesn't handle potential parsing errors and edge cases. If tokenization fails or if there are malformed Python files, this could raise exceptions.
Add error handling to make the function more robust:
def extract_inline_function_comments(source_code: str) -> dict[str, list[str]]: """ Finds *only* inline comments for function definition that match `SKIP_COMMENT` and returns them """ # Tokenize the source code to find comments - tokens = tokenize.generate_tokens(StringIO(source_code).readline) + try: + tokens = tokenize.generate_tokens(StringIO(source_code).readline) + except tokenize.TokenError: + LOGGER.warning("Failed to tokenize source code, skipping comment extraction") + return {} # To store the comments for each function prev_token = None comments = {} def_tok = False # Process the tokens and extract comments - for token in tokens: + try: + for token in tokens: tok_type, tok_string, _, _, _ = token # Detect the start of a new function definition if tok_type == tokenize.NAME and tok_string == "def": def_tok = True elif tok_type == tokenize.NAME and def_tok: # First "NAME" token after a "def" will be the function name prev_token = token def_tok = False elif tok_type == tokenize.NEWLINE and prev_token: # we found a function name and this is the first logical newline after it # if no comment has been found it means that anything that comes after could be within the function # or outside of it, which is outside the scope of what we are looking for. we can empty prev_token. # note that tokenize.NL would be a different (non-logical) newline, e.g. a multi-line function def # which is thus still handled correctly. # Not handling this here can cause comments outside the scope of the function to be mishandled, e.g. # ------------ # def foo(): # pass # # # my-comment # def bar(): # ------------ # would return "# my-comment" as a foo() comment prev_token = None # If this is the comment we look for, and it comes after a function definition elif tok_type == tokenize.COMMENT and prev_token and tok_string == SKIP_COMMENT: LOGGER.debug(f"found comment for function def: {prev_token.line.strip()}") LOGGER.debug(f"comment is: {tok_string}") func_name = prev_token.string comments[func_name] = [tok_string] prev_token = None + except Exception as e: + LOGGER.warning(f"Error extracting comments: {e}") + return {} return comments
119-121: Consider reusing file content instead of re-reading.The code reads the file twice: once for AST parsing and once for comment extraction. This could be inefficient for large files.
- with open(py_file) as fd: - tree = ast.parse(source=fd.read()) - - with open(py_file) as fd: - comments = extract_inline_function_comments(source_code=fd.read()) + with open(py_file) as fd: + source_code = fd.read() + + tree = ast.parse(source=source_code) + comments = extract_inline_function_comments(source_code=source_code)
148-151: Add a comment explaining the assert statement skip logic.The code skips functions that are only called from test assert statements, but there's no comment explaining this logic.
if _line.strip().startswith("assert"): - # if the function is only called from a test assert statement do not count it + # If the function is only called from a test assert statement, do not count it + # as being used, as it might only be referenced in tests and not actual code continuetests/unused_code/test_unused_code.py (2)
35-40: Enhance test assertions with explanatory messages.The test assertions would be clearer with explanatory messages to indicate what is being tested.
- # No function def that starts with "unused_code_" - assert ":unused_code_" not in result.output - assert "check_me" in result.output - assert "check_me_too" in result.output - assert "foo" not in result.output - assert "bar" not in result.output + # No function def that starts with "unused_code_" should be in the output + assert ":unused_code_" not in result.output, "Should not include functions with 'unused_code_' prefix" + # Functions with skip comments should not be in the output + assert "check_me" in result.output, "Should include 'check_me' as it's not properly skipped" + assert "check_me_too" in result.output, "Should include 'check_me_too' as it may not be properly skipped" + assert "foo" not in result.output, "Should not include 'foo' as it has a skip comment" + assert "bar" not in result.output, "Should not include 'bar' as it has a skip comment"
52-62: Add explicit test for the skip comment functionality.This test is good for verifying the skip comment functionality, but could be enhanced to explicitly check that the skipping is due to the comments.
Consider adding a more explicit test that verifies the debug output showing the skip comment message:
def test_skip_comment(): - result = get_cli_runner().invoke(get_unused_functions) + result = get_cli_runner().invoke(get_unused_functions, '-v') LOGGER.info(f"Result output: {result.output}, exit code: {result.exit_code}, exceptions: {result.exception}") assert result.exit_code == 1 assert "unused_code_check_fail" in result.output assert "unused_code_check_file" in result.output assert "check_me" in result.output assert "check_me_too" in result.output assert "foo" not in result.output assert "bar" not in result.output + # Verify debug messages indicating functions were skipped due to comments + assert "Skipping function due to comment: foo" in result.output + assert "Skipping function due to comment: bar" in result.output
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/unused_code/README.md(1 hunks)apps/unused_code/unused_code.py(4 hunks)tests/unused_code/test_unused_code.py(1 hunks)tests/unused_code/unused_code_file_for_test.py(1 hunks)
🧰 Additional context used
🪛 LanguageTool
apps/unused_code/README.md
[uncategorized] ~39-~39: Possible missing comma found.
Context: ...To skip single functions in your target repository you can add an inline comment to the fu...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~43-~43: Possible missing comma found.
Context: ...sed-code` ### Example: Given a target file main.py ```python def tmp1(): # skip-u...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
apps/unused_code/README.md
41-41: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tox
- GitHub Check: python-module-install
🔇 Additional comments (9)
apps/unused_code/README.md (1)
54-59: Great implementation of verbose output examples.Including the actual debug log examples showing what users will see when using the feature is very helpful for understanding the functionality.
tests/unused_code/unused_code_file_for_test.py (3)
12-13: Well-formatted skip comment.The skip comment is correctly formatted according to the implementation.
16-17: Good test case with type annotations.This provides a good test case for functions with type annotations and multiple parameters.
25-27:Details
❓ Verification inconclusive
Comment placement may cause issues.
The skip comment is placed on a separate line before the function definition, but the implementation in
extract_inline_function_commentsis designed to find inline comments that follow function definitions.This comment placement might not work as expected with the current implementation.
🏁 Script executed:
#!/bin/bash # Check if the function with comment on previous line gets skipped # Run the tool in verbose mode and check if check_me_too is skipped python -m apps.unused_code.unused_code -v | grep "check_me_too"Length of output: 386
Attention: Verify Skip Comment Behavior & Resolve Dependency Issue
- The tool’s current implementation in
extract_inline_function_commentsexpects skip comments to be inline (immediately following a function definition), but the skip comment intests/unused_code/unused_code_file_for_test.py(lines 25–27) is placed on the line preceding the function definition.- Note that the verification attempt failed due to a missing dependency (
simple_logger), which prevented a proper test run of this scenario. This dependency issue must be resolved before the skip-comment behavior can be reliably verified.- Action Items:
- Ensure the
simple_loggermodule is available in the testing environment so the tool can run without errors.- Re-verify that the skip comment placement is handled as expected. If the current inline detection is intended, consider placing the skip comment inline or modifying the detection logic to support preceding-line comments.
apps/unused_code/unused_code.py (3)
19-19: Good use of a constant for the skip comment.Defining the skip comment pattern as a constant makes it easier to maintain and update in the future.
111-114: Correctly implemented relative path check.The updated code properly checks relative paths against the file_ignore_list, which addresses the PR objective to fix relative paths for exclude-files.
157-164: Improved unused function reporting.The updated code now accumulates all unused functions and returns them at once, rather than returning after finding the first one. This provides a more comprehensive report to users.
tests/unused_code/test_unused_code.py (2)
16-19: Correctly updated to use relative path.The test now properly uses a relative path for the exclude-files option, which aligns with the PR objective to fix relative paths for exclude-files.
25-30: Good test for validating wrong path behavior.This test effectively validates that using an incorrect path format (just the filename instead of the relative path) doesn't exclude the file as expected.
Signed-off-by: lugi0 <lgiorgi@redhat.com>
Signed-off-by: lugi0 <lgiorgi@redhat.com>
|
|
||
| def process_file(py_file: str, func_ignore_prefix: list[str], file_ignore_list: list[str]) -> str: | ||
| if os.path.basename(py_file) in file_ignore_list: | ||
| if os.path.relpath(py_file) in file_ignore_list: |
There was a problem hiding this comment.
This may break existing usage. Expectation is, you would be running unused code check from root directory of your repo and if you want to ignore some file, you would be passing their names. Using relative file path can be problematic here.
There was a problem hiding this comment.
This is IMHO incorrect.
basename would return the tail of https://docs.python.org/3/library/os.path.html#os.path.split, which is assumed to be the file name of a given path.
This means that passing foo/xyz.py would return xyz.py, which might not be what the user has intended to exclude, e.g. in a scenario like
project/
├─ bar/
│ ├─ xyz.py
├─ foo/
│ ├─ xyz.py
├─ xyz.py
passing foo/xyz.py when running from the project root would actually exclude all xyz.py files (project/xyz.py, project/bar/xyz.py and project/foo/xyz.py).
relpath returns the relative path from the current directory - assuming you are excluding files based on their paths from the project root as is the current assumption, you would end up excluding the specific file rather than all of them
| with open(py_file) as fd: | ||
| tree = ast.parse(source=fd.read()) | ||
|
|
||
| with open(py_file) as fd: |
There was a problem hiding this comment.
The file was already opened in line 116, can you collect the comments there?
There was a problem hiding this comment.
No, file.read() returns the entire file contents and any subsequent read in the same context would return ''; the file has to be closed and reopened before its contents can be read again
| if _line.strip().startswith("#"): | ||
| continue | ||
|
|
||
| if _line.strip().startswith("assert"): |
There was a problem hiding this comment.
Can you please elaborate? Are we currently counting such calls?
There was a problem hiding this comment.
Currently the tool would count a function as being used even if its name was only referenced in an assert statement, e.g. see the test cases of this PR and imagine a scenario in which you are doing
assert "check_me" in output.
In this case "check_me" would suddenly stop being in the output, because the name of the function is now found, and even though the function itself is never actually used the tool incorrectly reports otherwise.
You can try running poetry run pytest with this line commented out to see it in action, but with this a function is only reported as used if it is actually called from some other place other than the assert statement(s).
As I wrote in the earlier comment it is an opinionated call, but I think it is fair to assume that the function name being found does not equal the function itself being used.
One could be more fancy with trying to figure out if the function name is encased in a string delimiter to rule out other such instances, but it would require a fair bit of work to get right
There was a problem hiding this comment.
Nice catch, I think the proper way t solve it is to use grep -E
git grep -wE '{func.name}(.*)' | grep -v 'def'
this will also replace
if f"def {func.name}" in _line: line
There was a problem hiding this comment.
The grep will not work, as it will still match the function name in any given context (e.g. print("my_function")). You would need to do negative lookbehind to ensure you're not in a string, but as the wise sage once said:
Some people, when confronted with a problem, think “I know, I’ll use regular expressions.” Now they have two problems.
The only way to properly handle it would be to special case it in the ast parse by matching the instance type with ast.Call, but even then I am not 100% sure of its correctness in all cases. Feel free to improve this as you see fit!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/unused_code/README.md (2)
37-40: Clarify Function Skipping Section
The new section clearly introduces how to skip individual functions via the# skip-unused-codeinline comment. To provide extra clarity, consider mentioning that the skip comment should be placed immediately before the function definition. This can help eliminate any ambiguity about its proper positioning.🧰 Tools
🪛 LanguageTool
[uncategorized] ~39-~39: Possible missing comma found.
Context: ...To skip single functions in your target repository you can add an inline comment to the fu...(AI_HYDRA_LEO_MISSING_COMMA)
41-42: Improve Markdown Heading Formatting
The heading "### Example:" includes a trailing colon, which does not comply with common markdown style guidelines (e.g., MD026 no-trailing-punctuation). It is recommended to remove the colon so that it reads simply "### Example".🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
41-41: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/unused_code/README.md(1 hunks)tests/unused_code/unused_code_file_for_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unused_code/unused_code_file_for_test.py
🧰 Additional context used
🪛 LanguageTool
apps/unused_code/README.md
[uncategorized] ~39-~39: Possible missing comma found.
Context: ...To skip single functions in your target repository you can add an inline comment to the fu...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~43-~43: Possible missing comma found.
Context: ...sed-code` ### Example: Given a target file main.py ```python def tmp1(): # skip-u...
(AI_HYDRA_LEO_MISSING_COMMA)
🪛 markdownlint-cli2 (0.17.2)
apps/unused_code/README.md
41-41: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: python-module-install
- GitHub Check: tox
🔇 Additional comments (2)
apps/unused_code/README.md (2)
43-53: Validate Inline Code Example
The Python example clearly demonstrates the intended usage of the# skip-unused-codecomment by annotating the functionstmp1andtmp2. The inline comment format matches the documented specification. Ensure that any potential issues raised by static analysis (e.g., a false positive about a missing comma) are verified to be non-issues.🧰 Tools
🪛 LanguageTool
[uncategorized] ~43-~43: Possible missing comma found.
Context: ...sed-code` ### Example: Given a target file main.py ```python def tmp1(): # skip-u...(AI_HYDRA_LEO_MISSING_COMMA)
54-60: Verify Output Example Consistency
The provided output sample correctly shows debug log entries for both functions being skipped. This output effectively illustrates the new behavior. Please double-check that this log format aligns with the overall tool’s logging conventions.
| SKIP_COMMENT = "# skip-unused-code" | ||
|
|
||
|
|
||
| def extract_inline_function_comments(source_code: str) -> dict[str, list[str]]: |
There was a problem hiding this comment.
found this to be more simple
https://pypi.org/project/ast-comments/
There was a problem hiding this comment.
Thanks for the suggestion! I wasn't familiar with this library, so I appreciate you bringing it up. My PR was more about implementing the logic directly, but if the preferred approach is to use that library instead, feel free to close this and go ahead with that direction.
There was a problem hiding this comment.
Why?
You can make this PR to use it.
There was a problem hiding this comment.
@lugi0
Please update if you continue with this PR.
I prefer you will :)
There was a problem hiding this comment.
@myakove as I said, if you wish to implement this functionality in a different way feel free to close the PR and move ahead with the proposed solution; you can also merge the PR and then update the code as you see fit, if that is preferable. I unfortunately do not have the time to reimplement the logic using a library I am not familiar with in the short term.
There was a problem hiding this comment.
@myakove seems interesting. I will explore it.
There was a problem hiding this comment.
@myakove I will. - if you don't already create a patch and fix it, before I get a chance!
| return "" | ||
|
|
||
| with open(py_file) as fd: | ||
| tree = ast.parse(source=fd.read()) |
There was a problem hiding this comment.
from ast_comments import parse
| tree = ast.parse(source=fd.read()) | |
| tree = parse(source=fd.read()) |
| if func.name in comments.keys(): | ||
| LOGGER.debug(f"Skipping function due to comment: {func.name}") | ||
| continue | ||
|
|
There was a problem hiding this comment.
if any(getattr(item, "value", None) == "# skip-unused-code" for item in func.body):
continue
|
added in #164 |
8 participants
Currently unused-code does not provide the ability to skip single function definitions, hence I've added the ability to skip via a specific comment added before the function definition (
# skip-unused-code).I've also fixed the behaviour of
--exclude-files, which currently only works with file names and excludes all files of a given name in a project (e.g.--exclude-files conftest.pywould exclude all conftest.py files in your project). Passing in relative paths to a specific file you want to exclude would be ignored, e.g.--exclude-files sub/dir/confest.pywould be ignored and the specific sub/dir/conftest.py would be checked for unused code.It now only excludes the file being specified, e.g.
--exclude-files conftest.pywould exclude conftest.py in the project root while--exclude-files sub/dir/confest.py(correctly) only excludessub/dir/confest.pySummary by CodeRabbit
New Features
# skip-unused-codecomment.Bug Fixes