txtorcon icon indicating copy to clipboard operation
txtorcon copied to clipboard

Use factory-methods to create instances

Open meejah opened this issue 9 years ago • 4 comments

Several classes use a Deferred attribute .post_bootstrap or similar so that calling code knows "when they're ready". This is pretty hacky and not very nice. JP Calderone has a good blog-post about this: http://as.ynchrono.us/2014/12/asynchronous-object-initialization.html

Instead:

  • a Deferred should be passed in to the constructor
  • factory methods (that return Deferreds/@inlineCallbacks) are provided
  • "normal" code calls the factory method

So, for example, instead of creating a TorConfig and waiting for .post_bootstrap code like this is preferred:

class TorConfig(object):
    @classmethod
    @inlineCallbacks
    def from_connection(cls, reactor, connection):
        bootstrapped = Deferred()
        cfg = TorConfig(..., bootstrapped)
        yield bootstrapped
        returnValue(cfg)

...so now "user" code would do this:

def main(reactor):
    con = yield TorControlProtocol.from_endpoint(reactor, TCP4ClientEndpoint("localhost", 9051))
    cfg = yield TorConfig.from_connection(reactor, con)
task.react(main)

meejah avatar Oct 09 '15 16:10 meejah

There are now TorState.from_protocol() and TorConfig.from_protocol(). Are there any others?

(The goal here is to elminate any need to mess with .post_bootstrap attributes ever, which is a bad pattern for initialization anyway).

meejah avatar Dec 28 '15 18:12 meejah

We've also added connect and launch which are nice high-level APIs where you don't muck about with .post_bootstrap either.

meejah avatar Mar 04 '17 17:03 meejah

The "real" way we should do this is to do away with ".bootstrap" altogether. The factory-functions should gather up any data required and pass it to the constructor for TorState etc. This would also make the tests way nicer as they don't need to make a fake protocol just to answer the "bootstrap" queries with the right fake data...

meejah avatar Mar 11 '17 18:03 meejah

This is partially done (there are factory methods) but we should also: deprecate .bootstrap and do the other refactorings mentioned (i.e. instead of TorState implementing its own "bootstrap" methods, we should query that stuff in the factory-function and pass in the data).

meejah avatar Apr 01 '18 07:04 meejah