To (monkey)patch or not to (monkey)patch, that is the question

[Aahz]

In article <1410d2e2-a6f2-4b6c-a745-6d3e34994568 at q16g2000yqq.googlegroups.com>,
>
George Sakkis  <george.sakkis at gmail.com> wrote:
>
>I was talking to a colleague about one rather unexpected/undesired
>(though not buggy) behavior of some package we use. Although there is
>an easy fix (or at least workaround) on our end without any apparent
>side effect, he strongly suggested extending the relevant code by hard
>patching it and posting the patch upstream, hopefully to be accepted
>at some point in the future. In fact we maintain patches against
>specific versions of several packages that are applied automatically
>on each new build. The main argument is that this is the right thing
>to do, as opposed to an "ugly" workaround or a fragile monkey patch.
>On the other hand, I favor practicality over purity and my general
>rule of thumb is that "no patch" > "monkey patch" > "hard patch", at
>least for anything less than a (critical) bug fix.
>
>So I'm wondering if there is a consensus on when it's better to (hard)
>patch, monkey patch or just try to work around a third party package
>that doesn't do exactly what one would like. Does it have mainly to do
>with the reason for the patch (e.g. fixing a bug, modifying behavior,
>adding missing feature), the given package (size, complexity,
>maturity, developer responsiveness), something else or there are no
>general rules and one should decide on a case-by-case basis ?

There's a related option that I chose for SQLObject: extending by using
internal APIs.  Because SQLite doesn't work well with multiple writers
(or even multiple readers with one writer when queries take a long time),
we needed a retry mechanism when a query failed.  I originally tried
proxying the SQLite class, but that ran into problems partly related to
new-style classes grabbing magic methods directly from the class, and
subclassing and using the internal registration API turned out to be
easier.  The only change required in the existing code was to change the
connection string to "sqlite_retry:".

Then I ran into build problems because some machines were on SQLObject
0.10.x and others were on 0.11.x and the internal API changed between
those versions.  It was easily fixed by upgrading everything to 0.11.x,
but it's a reminder about the fragility of relying on internal APIs.

If that's not an option in your case, personally I'd prefer
monkey-patching because otherwise I need to check the package into my
repository.  Submitting a patch upstream would be responsible behavior.

Overall, I think that this is definitely an issue where heuristics work
much better than rules.
-- 
Aahz (aahz at pythoncraft.com)           <*>         http://www.pythoncraft.com/

"At Resolver we've found it useful to short-circuit any doubt and just        
refer to comments in code as 'lies'. :-)"