Skip to content
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

Jump to bottom

bpo-40394 - difflib.SequenceMatched.find_longest_match default args #19742

Merged
merged 5 commits into from Apr 30, 2020
Merged

bpo-40394 - difflib.SequenceMatched.find_longest_match default args #19742

merged 5 commits into from Apr 30, 2020

Conversation

lrjball
Copy link
Contributor

@lrjball lrjball commented Apr 27, 2020 •
edited by bedevere-bot

Added default args to find_longest_match, as well as related tests

https://bugs.python.org/issue40394

@lrjball
bpo-40394 - difflib.SequenceMatched.find_longest_match default args
158712a 
Added default args to find_longest_match, as well as related tests
@the-knights-who-say-ni
Copy link

the-knights-who-say-ni commented Apr 27, 2020

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@lrjball

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@lrjball
Copy link
Contributor Author

lrjball commented Apr 27, 2020

CLA signed 👍

tim-one
tim-one requested changes Apr 28, 2020
View changes
Copy link
Member

@tim-one tim-one left a comment

Looks good so far! Needs more stuff:

  • Add your name to Misc/ACKS
  • Docs need to be updated to match, and with a versionchanged note.
  • Needs a NEWS entry.

Lib/test/test_difflib.py Show resolved Hide resolved
@bedevere-bot
Copy link

bedevere-bot commented Apr 28, 2020

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

blurb-it bot and others added 3 commits Apr 28, 2020
@blurb-it
📜🤖 Added by blurb_it.
977f922
@lrjball
Added docs, ACKS and more thorough tests
beb7ab3 
- Added docs to difflib.rst
- Added entry to ACKS
- Added more thorough testing by checking various combinations of the
args, and checking that it does give the actual longest match
@lrjball
Merge branch 'fix-issue-40394' of https://github.com/lrjball/cpython …
7669c02 
…into fix-issue-40394
@lrjball
Copy link
Contributor Author

lrjball commented Apr 28, 2020

I've made these updates, thanks. Worth mentioning as well that None is a valid param for ahi and bhi now, but will not work for alo and blo which still error if they aren't ints. Not sure if that should be documented anywhere or if it is fine how it is.

I have made the requested changes; please review again

@bedevere-bot
Copy link

bedevere-bot commented Apr 28, 2020

Thanks for making the requested changes!

@tim-one: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from tim-one Apr 28, 2020
@tim-one tim-one self-assigned this Apr 28, 2020
tim-one
tim-one requested changes Apr 29, 2020
View changes
Copy link
Member

@tim-one tim-one left a comment

Nice! I made one more code suggestion, which you should be able to commit directly from the comment. Or argue about it, but I don't suggest that 😉

Lib/difflib.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

bedevere-bot commented Apr 29, 2020

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@tim-one
Copy link
Member

tim-one commented Apr 29, 2020

None is a valid param for ahi and bhi now, but will not work for alo and blo which still error if they aren't ints. Not sure if that should be documented anywhere or if it is fine how it is.

It's fine. Nobody is going to pass an explicit None for any of these, and nothing in the docs or function signature suggest that passing None for alo or blo would make any sense.

@lrjball @tim-one
Update Lib/difflib.py
941a869 
Co-Authored-By: Tim Peters <tim.peters@gmail.com>
@lrjball
Copy link
Contributor Author

lrjball commented Apr 29, 2020

Thanks for the suggestion, I have added it in.

I have made the requested changes; please review again

@bedevere-bot
Copy link

bedevere-bot commented Apr 29, 2020

Thanks for making the requested changes!

@tim-one: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from tim-one Apr 29, 2020
tim-one
tim-one approved these changes Apr 30, 2020
View changes
Copy link
Member

@tim-one tim-one left a comment

All good - thank you, Lewis! I'll merge this now 😃

@tim-one tim-one merged commit 3209cbd into python:master Apr 30, 2020
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tim-one tim-one

Assignees

@tim-one tim-one

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lrjball @the-knights-who-say-ni @bedevere-bot @tim-one