bpo-16079: Add the duplicate_meth_defs.py tool as a pre-commit check on Travis #12886
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.
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.
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.
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.
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.pytool reports duplicate method definitions in a given list of files or directories. One must add an entry inTools/scripts/duplicates_ignored.txtto by-pass the pre-commit check when the reported duplicate is a false positive.https://bugs.python.org/issue16079