Opened 2 years ago

Last modified 16 months ago

#5781 enhancement new

Make something similar to _InstanceFactory public

Reported by: therve Owned by: Julian
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch:
Author: Launchpad Bug:

Description

For #5570, we need something that takes an existing protocol instance and returns it in buildProtocol. It looks a little bit like _InstanceFactory, except we may not care about having a deferred around, and it should be a "plain" server factory. Maybe we could base _InstanceFactory on the new class for example.

Attachments (1)

false-factory.patch (7.3 KB) - added by Julian 18 months ago.

Download all attachments as: .zip

Change History (6)

Changed 18 months ago by Julian

comment:1 Changed 18 months ago by Julian

  • Owner changed from therve to Julian

Not ready for review quite yet, but is this all you had in mind?

Here's a factory that does so, along with using it in a couple of places where the test suite was doing something similar. I don't see a way to simplify _InstanceFactory much using something like this.

I called it FalseFactory since I don't really like the name InstanceFactory. I don't really like FalseFactory too much either though :/, naming, it's hard.

comment:2 Changed 17 months ago by Julian

  • Keywords review added
  • Owner Julian deleted

comment:3 Changed 17 months ago by itamar

FalseFactory doesn't seem like an informative name (in what way is it false?). SingleInstanceFactory maybe?

comment:4 Changed 17 months ago by Julian

I don't like the name either, but it's false in the sense of "factories produce (new) stuff but this one does not", or something.

SingleInstanceFactory is not bad I guess. I almost called it SingletonFactory.

comment:5 Changed 16 months ago by exarkun

  • Keywords review removed
  • Owner set to Julian

Thanks for working on this one.

  1. I wonder if the behavior being asserted by test_alwaysBuildsTheSameProtocol actually makes sense. In general, it 'doesn't' make sense to try to re-use a IProtocol provider. makeConnection may only be called once. connectionLost may only be called once. I'd consider whether a better behavior for a second call to buildProtocol would be to raise an exception instead. Regardless, try to write test methods that only include one assertion.
  2. test_repr is missing a docstring.
  3. test_repr interpolates a Protocol instance into a string. Always interpolate a tuple, instead.
  4. Also, avoid string interpolation in unit tests for string formatting anyway. Comparing against a string literal produces a better test: it's easier to see what's going on, it's less likely there'll be some surprising mistake in the output.
  5. I agree that FalseFactory is not the best name for this functionality. SingleInstanceFactory seems fine to me. It makes it quite clear what the behavior is. Similar ideas: SingleProtocolFactory, SingleConnectionFactory, UseOnceFactory
  6. Please also update the client howto with some text explaining this class and giving an example of its use (add something to howto/listings/ if you want to add a runnable example, rather than examples/, please).

Also, please think for a minute about whether SingleProtocolFactory(protocolInstance) is the best API we can add right now. Would something like SomeFactory(callable, *args, **kwargs) (or just SomeFactory(callable)?) be a better addition? Maybe, or maybe that's just another API we should have separate from this one.

Thanks again.

Note: See TracTickets for help on using tickets.