-
Notifications
You must be signed in to change notification settings - Fork 2
Add ability to skip a single function def, fix relative paths for exclude-files in unused-code #156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ff18c6c
4bd8c31
9c2c334
6c04224
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,7 +5,9 @@ | |||||
| import os | ||||||
| import subprocess | ||||||
| import sys | ||||||
| import tokenize | ||||||
| from concurrent.futures import Future, ThreadPoolExecutor, as_completed | ||||||
| from io import StringIO | ||||||
| from typing import Any, Iterable | ||||||
|
|
||||||
| import click | ||||||
|
|
@@ -14,6 +16,60 @@ | |||||
| from apps.utils import ListParamType, all_python_files, get_util_config | ||||||
|
|
||||||
| LOGGER = get_logger(name=__name__) | ||||||
| SKIP_COMMENT = "# skip-unused-code" | ||||||
|
lugi0 marked this conversation as resolved.
|
||||||
|
|
||||||
|
|
||||||
| def extract_inline_function_comments(source_code: str) -> dict[str, list[str]]: | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. found this to be more simple
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lugi0
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @myakove seems interesting. I will explore it.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @myakove I will. - if you don't already create a patch and fix it, before I get a chance! |
||||||
| """ | ||||||
| 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) | ||||||
|
|
||||||
| # To store the comments for each function | ||||||
| prev_token = None | ||||||
| comments = {} | ||||||
| def_tok = False | ||||||
|
|
||||||
| # Process the tokens and extract comments | ||||||
| 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 | ||||||
|
|
||||||
| return comments | ||||||
|
|
||||||
|
|
||||||
| def is_fixture_autouse(func: ast.FunctionDef) -> bool: | ||||||
|
|
@@ -53,14 +109,22 @@ def is_ignore_function_list(ignore_prefix_list: list[str], function: ast.Functio | |||||
|
|
||||||
|
|
||||||
| 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: | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is IMHO incorrect. passing |
||||||
| LOGGER.debug(f"Skipping file: {py_file}") | ||||||
| return "" | ||||||
|
|
||||||
| with open(py_file) as fd: | ||||||
| tree = ast.parse(source=fd.read()) | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| with open(py_file) as fd: | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The file was already opened in line 116, can you collect the comments there?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No,
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. save |
||||||
| comments = extract_inline_function_comments(source_code=fd.read()) | ||||||
|
|
||||||
| found = [] | ||||||
| for func in _iter_functions(tree=tree): | ||||||
| if func.name in comments.keys(): | ||||||
| LOGGER.debug(f"Skipping function due to comment: {func.name}") | ||||||
| continue | ||||||
|
|
||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
| if func_ignore_prefix and is_ignore_function_list(ignore_prefix_list=func_ignore_prefix, function=func): | ||||||
| LOGGER.debug(f"Skipping function: {func.name}") | ||||||
| continue | ||||||
|
|
@@ -81,12 +145,23 @@ def process_file(py_file: str, func_ignore_prefix: list[str], file_ignore_list: | |||||
| if _line.strip().startswith("#"): | ||||||
| continue | ||||||
|
|
||||||
| if _line.strip().startswith("assert"): | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please elaborate? Are we currently counting such calls?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently the tool would count a function as being used even if its name was only referenced in an
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice catch, I think the proper way t solve it is to use this will also replace
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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! |
||||||
| # if the function is only called from a test assert statement do not count it | ||||||
| continue | ||||||
|
|
||||||
| if func.name in _line: | ||||||
| used = True | ||||||
| break | ||||||
|
|
||||||
| if not used: | ||||||
| return f"{os.path.relpath(py_file)}:{func.name}:{func.lineno}:{func.col_offset} Is not used anywhere in the code." | ||||||
| # store all unused functions in the file | ||||||
| found.append( | ||||||
| f"{os.path.relpath(py_file)}:{func.name}:{func.lineno}:{func.col_offset} Is not used anywhere in the code.\n" | ||||||
| ) | ||||||
|
|
||||||
| # return all unused functions if any | ||||||
| if len(found) > 0: | ||||||
| return "".join(found) | ||||||
|
|
||||||
| return "" | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,31 @@ | ||
| from typing import Any | ||
|
|
||
|
|
||
| def unused_code_check_fail(): | ||
| pass | ||
|
|
||
|
|
||
| def unused_code_check_file(): | ||
| pass | ||
|
|
||
|
|
||
| def foo(): # skip-unused-code | ||
| pass | ||
|
|
||
|
|
||
| def bar( | ||
| x: Any, | ||
| y: Any, | ||
| z: Any | ||
| ) -> None: # skip-unused-code | ||
| pass | ||
|
|
||
|
|
||
| def check_me(): | ||
| # skip-unused-code | ||
| pass | ||
|
|
||
|
|
||
| # skip-unused-code | ||
| def check_me_too(): | ||
| pass |
Uh oh!
There was an error while loading. Please reload this page.