GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-16079: Add the duplicate_meth_defs.py tool as a pre-commit check on Travis by xdegaye · Pull Request #12886 · python/cpython

Conversation

@xdegaye

@xdegaye xdegaye commented Apr 20, 2019 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@xdegaye
Add the duplicate_meth_defs.py tool as a pre-commit check on Travis
f450f09 
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.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Apr 20, 2019
@bedevere-bot bedevere-bot added the awaiting review label Apr 20, 2019
tirkarthi
tirkarthi reviewed Apr 21, 2019
View reviewed changes
Comment thread Tools/scripts/duplicate_meth_defs.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
xdegaye added 3 commits April 21, 2019 18:29
@xdegaye
Handle AsyncFunctionDef nodes same as FunctionDef nodes
ea41f06
@xdegaye
Merge remote-tracking branch 'upstream/master' into bpo-16079
45c5e3c
@xdegaye
Remove duplicates that have been fixed.
c5c30ad
@ned-deily ned-deily mentioned this pull request May 29, 2019
[3.7] bpo-16079: Add the duplicate_meth_defs.py tool as a pre-commit check on Travis (GH-12886) #12950
Closed
@ned-deily

ned-deily commented Sep 10, 2019 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gpshead
gpshead reviewed Sep 10, 2019
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/scripts/patchcheck.py
import untabify
from io import StringIO
from test.support import change_cwd
from duplicate_meth_defs import main as find_duplicates

@gpshead gpshead Sep 10, 2019

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/scripts/patchcheck.py
fn.endswith(('.rst', '.inc'))]
got_exception, duplicates = duplicates_found(python_files)
if duplicates:
print('Please fix the duplicate method definitions found')

@gpshead gpshead Sep 10, 2019

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@xdegaye xdegaye Sep 11, 2019

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@xdegaye xdegaye closed this Dec 10, 2019
@xdegaye
xdegaye deleted the bpo-16079 branch December 10, 2019 12:22
@xdegaye xdegaye mannequin mentioned this pull request Jun 27, 2023
list duplicate test names with patchcheck #60283
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead left review comments
@tirkarthi tirkarthi tirkarthi left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@xdegaye @ned-deily @gpshead @tirkarthi @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.