On Fri, Jan 13, 2017 at 1:09 AM, Cory Benfield <cory@lukasa.co.uk> wrote:
On 13 Jan 2017, at 09:07, Nathaniel Smith <njs@pobox.com> wrote:
I was imagining something along the lines of
ClientContext({...}).wrap_socket(...)
where ClientContext.__init__ would do the validation. Or there are various other ways to slot the pieces together, but in general validation doesn't need to be done incrementally; there's going to be some place where the config dict hits the library and it can/should be validated at that point. And before that point we get all the dict niceties for free.
What are the dict niceties, exactly? I’m still not seeing what the advantage is in throwing away a typed and restricted API for a dict is.
Getters & setters (right now the spec has setters for everything, but a getter for validate_certificates only -- that's kinda weird?), an obvious serialization story for the parts of the config that use basic types (e.g. to/from JSON/TOML/YAML), an obvious/natural way to represent "here are the config changes I want to make in response to that ClientHello" for the sni callback as a declarative object, a nice __repr__, familiarity, ... obviously these are all things one can put into the spec piece by piece, but you're the one who said you wanted to simplify things :-). I get what you're saying about a typed API -- basically in the current setup, the way _BaseContext is written as a bunch of Python methods means that the interpreter will automatically catch if someone tries to call set_cihpers, whereas in the dict version each implementation would have to catch this itself. But in practice this difference seems really minimal to me -- either way it's a runtime check, and it's really difficult to write a loop like for key, value in config.items(): do_some_quirky_cffi_thing_based_on_key() that doesn't implicitly validate the keys anyway. There's also the option of using e.g. JSON-Schema to write down a formal description of what goes in the dict -- this could actually be substantially stricter than what Python gets you, because you can actually statically enforce that validate_certificates is a bool. For whatever that's worth -- probably not much. If we really want to simplify it down, then as Alex noted in the PR, TLSWrappedSockets are superfluous, so we could make it just: # Simple functions, not methods: client_wrap_buffers(config, incoming, outgoing) -> TLSWrappedBuffers server_wrap_buffers(config, incoming, outgoing) -> TLSWrappedBuffers class TLSWrappedBuffers: ... read and all that stuff ... @property @abstractmethod def get_config(self) -> dict: pass def update_config(self, new_settings): # Attempt to do the equivalent of self.config.update(new_settings); # raises TLSError if any of the settings can't be changed given # current backend or current connection state. Anyway, I'm not actually sure if this is a good idea, but it seemed worth putting the two approaches up where we could look at them and compare :-). -n -- Nathaniel J. Smith -- https://vorpus.org