[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Irit Katriel]

Change by Irit Katriel :


--
stage: patch review -> resolved
status: pending -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Terry J. Reedy]

Terry J. Reedy  added the comment:

'Pending' is pretty useless because any comment erases it.  This is a tracker 
bug that will not be fixed.

--
status: open -> pending

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Terry J. Reedy]

Terry J. Reedy  added the comment:

I reread the patch and my previous comment and am no more positive than I was 
then.

The one use case presented is second-hand.  Chris said "in order for [Bazaar] 
to use a custom sequence matcher, they had to essentially copy-paste and modify 
the stdlib diff methods in order to inject their own sequence matchers."  
Assuming that they did something like the patch, it is not clear why they did 
not instead patch/replace SequenceMatcher, and they are not involved here to 
speak.

Enhancements need to be of some general use with multiple use cases or users.  
When I added the SequenceMatcher autojunk parameter in 3.2 to disable the 
autojunk heuristic, there had been multiple reports of how it caused problems.  
There has been no additional support given here, including none from Tim.  One 
place to get other on board for enhancements is python-idea list.

--
assignee: tim.peters -> 
status: pending -> open

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Irit Katriel]

Irit Katriel  added the comment:

I agree with Terry that this proposal significantly complicates the API and the 
motivation for it is not very strong. I will close the issue in a couple of 
weeks if nobody objects.

--
nosy: +iritkatriel
resolution:  -> rejected
status: open -> pending

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Claudiu Popa]

Changes by Claudiu Popa pcmantic...@gmail.com:


--
nosy:  -Claudiu.Popa

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Chris Rose]

Chris Rose added the comment:

What's the word on this change?

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Terry J. Reedy]

Terry J. Reedy added the comment:

Current status: Tim is currently not actively involved in Python development 
and no one who is active is enthusiastic about the proposal.

Your proposal is to add dependency injection to the initializer for the 4 diff 
classes by adding 6 new parameters. I will comment on the proposal itself 
below.  But first...

Your initial message in support of the proposal was about as weak as possible: 
it would be possible to   Lots of things are possible. For instance, we 
could add a Socket class replacement parameter to all stdlib functions and 
classes that use socket.Socket (and this has been requested in at least one 
case).  However, only a few changes actually get done.  Recognizing this ...

Your later message gave one use case: Bazaar's experiments with an alternate 
implementation.  To me, this example is still inadequate justification for a 
change. We agree that cut-and-paste is bad (though not terrible for one-off 
experiments).  That is why Python allows dynamic patching ('monkeypatching') of 
modules and Python-coded classes as an alternative (though subclassing is often 
better for classes).

import difflib
# either
class MyDiff: pass  # not really ;-)
# MyDiff might or might not subclass SequenceMatcher
difflib.SequenceMatcher = MyDiff
# or
def f(self, *args): pass  # again, not really
difflib.SequenceMatcher.somemethod = f
# either way, use difflib functions with alternate matcher.

I consider subclassing + dynamic patching an adequate solution* for the one use 
case you mention and likely other uses.  So I am inclined to reject the 
proposal as 'unnecessary'.  (If multiple people were routinely monkeypatching 
difflib like this, I might change my mind.)

* A context manager can be used to make the monkeypatching temporary and 
therefore local in effect. The following works.

@contextlib.contextmanager
def mp(ob, attr, new):
old = getattr(ob, attr)
setattr(ob, attr, new)
yield
setattr(ob, attr, old)


As for the proposal itself: you mention possible API alternatives.  I am more 
concerned about expanding our responsibility and corresponding test 
requirements.  If you monkeypatch the module, then you are responsible for the 
results.  If we add what amount to monkeypatching parameters, then we become 
responsible.  We would have to make sure that the requirements for replacements 
are adequately defined and design at least one non-trivial replacement and run 
all the appropriate tests with that replacement.  Your patch does not do this.  
I would not want to do this. We would also have to be prepared to maintain the 
expanded definition of the classes.

Extending the mandate of the four classes from work reasonably well with the 
builtin diff engine to work reasonably well with any 'similar' diff engine 
passed in by a user is not as trivial as implied by the patch.

For a simple function like filter, the requirement for the function parameter 
is simple.  It must be a predicate with respect to the items yielded by the 
iterable arguments.  That means returning False or True and never raising when 
applied to each item.  It is easy to create alternatives such as lambda x: 
False, lambda x: True, lambda x: bool(x), and lambda x: x  0, and test with 
appropriate iterables.

I hope this explains a bit why function parameters are routine in Python while 
class parameters are much less common.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Claudiu Popa]

Changes by Claudiu Popa pcmantic...@gmail.com:


--
stage:  - patch review

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Chris Rose]

Chris Rose added the comment:

As suggested: SYN

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[R. David Murray]

R. David Murray added the comment:

Well, it would be 3.5, so there's plenty of time yet.  We are hoping for an 
answer from Tim Peters, but if we don't get one someone else will review it as 
time allows.  Pinging the issue like this was good; do it again in another 
month if there's no further action before then.  (NB: we're working on 
improving our workflow so that issues like this don't get lost, but it will 
take time for those improvements to show up...)

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Chris Rose]

Chris Rose added the comment:

Ping? I'd like to know if the proposed solution passes muster, so that I can 
get this into ... well, whatever the right revision would be.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Chris Rose]

Changes by Chris Rose off...@offby1.net:


--
nosy: +r.david.murray

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Chris Rose]

Chris Rose added the comment:

Updated according to review.

--
Added file: http://bugs.python.org/file34868/difflib-sm.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Claudiu.Popa]

Claudiu.Popa added the comment:

Hi. I added a couple of comments for your previous patch, the new one doesn't 
seem to have a review link.

--
nosy: +Claudiu.Popa

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Chris Rose]

Chris Rose added the comment:

Addressed comments regarding documentation and assertion formats in the test.

--
Added file: http://bugs.python.org/file34869/difflib-sm.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Chris Rose]

Chris Rose added the comment:

Patch against tip

--
Added file: http://bugs.python.org/file34871/difflib-sm.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Chris Rose]

Chris Rose added the comment:

As a historical record, it should be noted that this is driven by an actual use 
case: I was experimenting with using Bazaar's patience diff implementation, and 
I saw that in order for them to use a custom sequence matcher, they had to 
essentially copy-paste and modify the stdlib diff methods in order to inject 
their own sequence matchers. That struck me as a bad thing, and that's pretty 
much what led to this.

I welcome a discussion of the API itself; there's definitely a bit of an odd 
challenge in describing the usage of the matcher variants when both are used 
(in line_matcher and char_matcher roles).

A possible approach would be to consider matcher factories to take _just_ a 
junk function, nothing else, and use the SequenceMatcher API's set_seqs method 
to actually provide the sequences in all cases. This fits the character use 
case, which reuses the matcher, and the line use case which does not.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Terry J. Reedy]

Changes by Terry J. Reedy tjre...@udel.edu:


--
nosy: +terry.reedy

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Raymond Hettinger]

Raymond Hettinger added the comment:

Uncle Timmy, what say you?

--
assignee:  - tim.peters
nosy: +rhettinger, tim.peters

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Chris Rose]

New submission from Chris Rose:

It should be possible to provide subclasses or implementations of the 
SequenceMatcher interface to difflib's various methods that do formatting (for 
example, unified_diff/context_diff et al).

I've included a patch for it that satisfies my personal itch on the subject 
(attached). I'd like to have this reviewed for inclusion in 3.5.

--
components: Library (Lib)
files: difflib-sm.patch
keywords: patch
messages: 212036
nosy: offby1
priority: normal
severity: normal
status: open
title: Difflib should provide the option of overriding the SequenceMatcher
type: enhancement
versions: Python 3.5
Added file: http://bugs.python.org/file34203/difflib-sm.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue20752] Difflib should provide the option of overriding the SequenceMatcher

[Chris Rose]

Chris Rose added the comment:

I've regenerated the patch with doc additions and Misc/ACKS changed as well.

--
Added file: http://bugs.python.org/file34204/difflib-sm.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue20752
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com