On Tue, 2014-11-18 at 09:35 -0500, Rafael Schloming wrote: > On Mon, Nov 17, 2014 at 4:48 PM, Andrew Stitcher <[email protected]> > wrote: > ... > There are a couple things I don't like about the API after the change. > > 1. pn_transport_client()/pn_transport_server() don't follow our naming > conventions, as constructors you would expect them to return distinct > types, however they both return pn_transport_t.
I'd prefeer to return 2 distinct types which are subtypes of a common base. However the type system of C doesn't allow me to change the code safely to do this and keep the same common code. Now that you mention it I could make typedefs: typedef pn_transport_client_t pn_transport_t; typedef pn_transport_server_t pn_transport_t; And still be able to safely use the existing pn_transport_xxx() APIs. > 2. Given that we have a distinct configuration phase regardless, it is > simply unnecessary to force people to choose client vs server up front. We > can simply default the mode to client and have people set it to server > during the configuration phase if that is what they wish. As clients and servers are fundamentally different in usage it makes no semantic sense to me to create a client then "turn it into" a server. > ... > As a general pattern, I prefer to stick with (2). I think it's more > flexible/extensible and allows the addition of pretty much arbitrary > configuration/construction parameters without ever actually breaking the > API. As a general pattern, the alternative is IMHO really quite brittle, > e.g. if we have another parameter we want to add in the next release we > need to choose between a combinatorial explosion of constructors, or > breaking the API yet again: > > pn_transport_websockets_server(); // confusing/bloated API > pn_transport(SERVER, WEBSOCKETS); // breaks everytime we add a parameter This is a strawman. I'm not suggesting that non-fundamental transport attributes like the layers present or their configuration be added to the constructor. But I am trying remove unneeded configuration from various IO layers if it can be sensibly defaulted. And in the case of implementing something like websockets (assuming it would be a layer) you would add it in the same way as ssl/sasl - viz pn_websocket(pn_transport_t*); This should figure out if it as a client or server side by getting that from the transport, possibly with a way to override the default if (and only if) that is required. > I think if you insist upon breaking the API then option (1) is preferable > to what you currently have, however as I said above, I don't believe there > is really any reason to break the API. I don't insist on it - I can make sure the one non-working case works: [That's: t = pn_transport(); pn_sasl(t); pn_sasl_server(); ] Andrew > > --Rafael --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
