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

difflib.SequenceMatcher.find_longest_match default arguments #84574

Closed
lrjball mannequin opened this issue Apr 26, 2020 · 9 comments
Closed

difflib.SequenceMatcher.find_longest_match default arguments #84574

lrjball mannequin opened this issue Apr 26, 2020 · 9 comments
Assignees
@tim-one
Labels
3.9 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@lrjball
Copy link
Mannequin

lrjball mannequin commented Apr 26, 2020

BPO 40394
Nosy @tim-one, @lrjball
PRs
  • bpo-40394 - difflib.SequenceMatched.find_longest_match default args #19742
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/tim-one'
closed_at = <Date 2020-04-30.03:45:43.891>
created_at = <Date 2020-04-26.14:15:58.546>
labels = ['type-feature', 'library', '3.9']
title = 'difflib.SequenceMatcher.find_longest_match default arguments'
updated_at = <Date 2020-05-02.00:12:06.898>
user = 'https://github.com/lrjball'

    bugs.python.org fields:

    activity = <Date 2020-05-02.00:12:06.898>
actor = 'Lewis Ball'
assignee = 'tim.peters'
closed = True
closed_date = <Date 2020-04-30.03:45:43.891>
closer = 'tim.peters'
components = ['Library (Lib)']
creation = <Date 2020-04-26.14:15:58.546>
creator = 'Lewis Ball'
dependencies = []
files = []
hgrepos = []
issue_num = 40394
keywords = ['patch']
message_count = 9.0
messages = ['367306', '367380', '367456', '367459', '367461', '367464', '367731', '367732', '367902']
nosy_count = 2.0
nosy_names = ['tim.peters', 'Lewis Ball']
pr_nums = ['19742']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue40394'
versions = ['Python 3.9']
    @lrjball
    Copy link
    Mannequin Author

    lrjball mannequin commented Apr 26, 2020

    The usage of difflib.SequenceMatcher.find_longest_match could be simplified for the most common use case (finding the longest match between the entirety of the two strings) by taking default args.

    At the moment you have to do:

    >>> from difflib import SequenceMatcher
>>> a, b = 'foo bar', 'foo baz'
>>> s = SequenceMatcher(a=a, b=b)
>>> s.find_longest_match(0, len(a), 0, len(b))
Match(a=0, b=0, size=6)

    but with default args the final line could be simplified to just:

    >>> s.find_longest_match()
Match(a=0, b=0, size=6)

    which seems to be much cleaned and more readable.

    I'd suggest updating the code so that the function signature becomes:

    find_longest_match(alo=None, ahi=None, blo=None, bhi=None)

    which is consistent with the current docstring of "Find longest matching block in a[alo:ahi] and b[blo:bhi]." as a[None:None] is the whole of a.

    I think this would only be a minor code change, and if it is something that would be useful I'd be happy to have a go at a PR.

    @lrjball lrjball mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Apr 26, 2020
    @tim-one
    Copy link
    Member

    tim-one commented Apr 27, 2020

    Sounds good to me, Lewis - thanks! Note, though, that alo and blo should default to 0. None is best reserved for cases where the default value needs to be computed at runtime. But alo == blo == 0 apply to all possible instances.

    @lrjball
    Copy link
    Mannequin Author

    lrjball mannequin commented Apr 27, 2020

    Okay, that makes sense. I will raise a PR

    @lrjball
    Copy link
    Mannequin Author

    lrjball mannequin commented Apr 27, 2020

    Adding a test for this and noticed I can add one more test case to get the method to full coverage. Can I add that to this PR or should I raise a separate one?

    @tim-one
    Copy link
    Member

    tim-one commented Apr 27, 2020

    I'm not clear on exactly what it is you're asking, but it's better to ask for forgiveness than permission ;-) That is, it's unlikely anyone will object to adding a test in a feature PR.

    @tim-one tim-one added 3.9 only security fixes labels Apr 27, 2020
    @lrjball
    Copy link
    Mannequin Author

    lrjball mannequin commented Apr 27, 2020

    Oh okay, well I was just saying I have added a test which is unrelated to the feature I have added, but it does test a different part of the same function. Anyway, I have raised a PR for this now (19742) and can separate it out if needed.

    @tim-one
    Copy link
    Member

    tim-one commented Apr 30, 2020

    New changeset 3209cbd by lrjball in branch 'master':
    bpo-40394 - difflib.SequenceMatched.find_longest_match default args (GH-19742)
    3209cbd

    @tim-one
    Copy link
    Member

    tim-one commented Apr 30, 2020

    All done. Thank you, Lewis! You're now an official Python contributor, and are entitled to all the fame, fortune, and power that follows. Use your new powers only for good :-)

    @tim-one tim-one closed this as completed Apr 30, 2020
    @tim-one tim-one closed this as completed Apr 30, 2020
    @lrjball
    Copy link
    Mannequin Author

    lrjball mannequin commented May 2, 2020

    Thanks Tim. Cheers for your support with this :)

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @tim-one tim-one

    Labels
    3.9 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    1 participant
    @tim-one