8000 bpo-16079: Add the duplicate_meth_defs.py tool as a pre-commit check on Travis by xdegaye · Pull Request #12886 · python/cpython · GitHub
[go: up one dir, main page]

Skip to content

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

Closed
wants to merge 4 commits into from
Closed

Conversation

xdegaye
Copy link
Contributor
@xdegaye xdegaye commented Apr 20, 2019

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.

https://bugs.python.org/issue16079

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.
@ned-deily
Copy link 8000
Member
ned-deily commented Sep 10, 2019

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.

Copy link
Member
@gpshead gpshead left a 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
Copy link
Member

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')
Copy link
Member

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.

Copy link
Contributor Author

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.

@xdegaye xdegaye closed this Dec 10, 2019
@xdegaye xdegaye deleted the bpo-16079 branch December 10, 2019 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0