-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
bpo-16079: Add the duplicate_meth_defs.py tool as a pre-commit check on Travis #12886
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
Conversation
The duplicate_meth_defs.py tool reports duplicate method definitions in a given list of files or directories. One must add an entry in `Tools/scripts/duplicates_ignored.txt` to by-pass the pre-commit check when the reported duplicate is a false positive.
See bpo-16079 discussion. Since this PR is requesting a change in our development process, I've added some people to the bpo issue in the hopes of reaching a decision about whether we want to proceed with adding this to the current CI testing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall: This is a great check to have in any code base. duplicate def's are a common problem, especially in unittests where authors often start by cut and pasting an existing test.
mostly lets make sure the implementation is not a pain. i don't think it will be.
|
||
import reindent | ||
import untabify | ||
from io import StringIO | ||
from test.support import change_cwd | ||
from duplicate_meth_defs import main as find_duplicates |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it a tool or a library? We're only using it as a library. Lets design it as a library instead of treating main as an API with a stubbed out stdout.
@@ -230,6 +260,9 @@ def travis(pull_request): | |||
c_files = [fn for fn in file_paths if fn.endswith(('.c', '.h'))] | |||
doc_files = [fn for fn in file_paths if fn.startswith('Doc') and | |||
fn.endswith(('.rst', '.inc'))] | |||
got_exception, duplicates = duplicates_found(python_files) | |||
if duplicates: | |||
print('Please fix the duplicate method definitions found') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message needs to link to instructions onto how to do so, more specifically including how to update the list of things that should be ignored when that is the solution (when it isn't I expect people to already figure out the problem by the
As implemented, this could use the ast
module from a potentially different version of python3 than the tree being compiled uses. (unless we run patchcheck using our own interpreter? i haven't looked)
Which means some test files are going to contain syntax that does not parse at various times. Those should not be things to list in an exception file. Ideally we want such a tool to run under own interpreter so that it's ast is our ast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we want such a tool to run under own interpreter so that it's ast is our ast.
IMHO it should be more restrictive. The interpreter used to run this tool must be built from the head of the master branch to handle the case where the grammar of the Python language is being changed and the tool is run on a file that tests this change which would raise a syntax error when compiled with an interpreter that does not support the new grammar change.
There is a similar issue when cross-compiling where a native interpreter is used during the cross-compilation to generate the sysconfig build-time data with 'generate-posix-vars' or to run setup.py. This problem actually occured when f-string where introduced in 3.6.
The
duplicate_meth_defs.py
tool reports duplicate method definitions in a given list of files or directories. One must add an entry inTools/scripts/duplicates_ignored.txt
to by-pass the pre-commit check when the reported duplicate is a false positive.https://bugs.python.org/issue16079