[devel at bb.net] Use of module wrapping in bug2340

[Vladimir Rutsky]

Hello!

During "slave"->"worker" transition in bug 2340 names such as 
buildbot.locks.SlaveLock
will be renamed to "worker"-based names, e.g. buildbot.locks.WorkerLock.
For some classes fallback will be provided, e.g. 
buildbot.locks.SlaveLock will still
be available in 0.9.0, however it's use will emit deprecation warning.

My initial fallback implementation idea (that I expressed in PR #1943 [1])
were based on an opinion, that Python module replacement in sys.modules 
is a terrible hack
and should be avoided at all costs.

So to add fallback for buildbot.locks.SlaveLock I added dummy class via 
wrapper
that were named SlaveLock, inherited from WorkerLock and has overriden 
__new__
to emitted warning, when SlaveLock is *instantiated*.
Effectively:

class WorkerLock:
     ...

class SlaveLock(WorkerLock):
def __new__(cls, *args, **kwargs):
         # Raise warning here
         return cls.__new__(instance_cls)


To add fallback for a function I added dummy function that emitted warning
*when called* and forwarded all arguments to the wrapped function.
Effectively:

def enforceChosenWorker(bldr, workerforbuilder, breq):
     ...

def enforceChosenSlave(*args, **kwargs):
     # Raise warning here
     return enforceChosenWorker(*args, **kwargs)

Recently I noticed that Twisted already has helper for deprecating 
module attributes [2]:
deprecatedModuleAttributes().
Twisted does it by replacing module in sys.modules with custom proxy 
object that
emits warnings when deprecated attribute is accessed.

I want to refactor my fallback helpers to use Twisted's approach (I will 
to use
deprecatedModuleAttributes in my wrappers) and interested in other 
developers
opinions regarding Twisted's approach.

Pros of using module proxy:

1. Function or class *access* will raise warning.
This is the only option to raise warnings in some cases, e.g. in case if 
user catches
BuildSlaveTooOldError in his code (that class is never instantiated in 
user code).

Cons of using module proxy:

1. It's a hack that may lead to unexpected behaviour.
If Twisted developers uses it I want to believe that this is not so 
terrible hack,
but at least cctrial has some issues on such modules [3]
(module proxy can not be reloaded).

2. It may make run code slower (insignificantly, IMO).

3. It doesn't scale. If anyone else will decide to replace module with 
it's own
wrapper it may result in a conflict.

4. It will emit warnings if star imports are used, even if no 
"slave"-named objects
are actually used (e.g. `from buildbot.interfaces import *` will emit 
warning).

Which implementation is more preferable?

I like module replacement approach, but afraid of star imports in the 
client code
(star import from buildbot.plugins *will* work properly without warnings 
with my
current implementation of fallback in plugins, but not from other modules).

Switch implementation it's not a problem (API of my helpers will not 
change,
only their implementation).

[1] https://github.com/buildbot/buildbot/pull/1943
[2] 
https://twistedmatrix.com/documents/9.0.0/api/twisted.python.deprecate.html
[3] https://github.com/tardyp/cctrial/issues/4


Regards,

Vladimir Rutsky

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.buildbot.net/pipermail/devel/attachments/20160203/9685dc07/attachment.html>