> On Jan. 27, 2015, 7:37 p.m., Rafael Schloming wrote: > > proton-c/bindings/python/proton/__init__.py, line 2702 > > <https://reviews.apache.org/r/30318/diff/1/?file=836290#file836290line2702> > > > > This could be a bit nicer if we were to check for an encode method or > > something rather than hardcode an isinstance test against Message. That way > > I could implement arbitrary objects that can be encoded as AMQP messages.
Yes, that would be an easy change > On Jan. 27, 2015, 7:37 p.m., Rafael Schloming wrote: > > proton-c/bindings/python/proton/__init__.py, line 2701 > > <https://reviews.apache.org/r/30318/diff/1/?file=836290#file836290line2701> > > > > This seems to change the semantics of send in a subtle but odd way. If > > you pass in something that it knows how to encode into bytes, it will > > assume that constitutes an entire message and call advance and > > automatically generate a tag for you. Yet if you pass in raw bytes, it > > doesn't do this. I think these two things should really be controlled > > orthogonally, e.g. I might want to be able to configure the sender to > > assume every call to send is a single message even if I'm just passing in > > raw bytes, and I might also want to be able to stream large messages out by > > passing in higher level message chunks that the sender automatically > > encodes into bytes for me. WHat I want is a simple, single call to send a message. I'd like that to be called send(), since that seems a clear and intuitive name for it. However that clashes with the existing definition. I would argue that send() isn't such a great name for the current semantics in that it isn't (in my view) clear that you first need to create a delivery and then need to call advance() when done. To me the current semantic is more like an append_data(). However, I didn't want to impose a breaking change. Hence the somewhat odd combination of semantics for the same name. There are (at least) three ways to deal with this: (1) use send_message() for the new simple, single call semantic and leave send() as is. I could live with that but it seems a shame to me to use the obvious name for the less obvious (and I believe less commonly required) semantic (2) use send() exclusively for the new simple, single call semantic, and have send_bytes() - or better still perhaps, append_data() - as the old semantic. This is obviously a breaking API change though, not sure how acceptable that would be? (3) add in some flag(s)/option(s) to control the single-shot v. part-of-sequence-of-calls semantics. However the defaults need to be decided. So agin, we can default it to the current bheaviour, but that in my view is less intuitive and less commonly what users want. Or we can default it to the new behaviour, but that then is another breaking change. To me, if a breaking change is acceptable, I'd probably go for something like (2), because I think it will be clearer. Perhaps I'm just not imaginative enough yet on what the flags/options might be like in 3. If a breaking change isn't acceptable, I'd probably rather just stick with a send_message() rather than have a send() where a non-default option was required to trigger the single-shot behaviour. > On Jan. 27, 2015, 7:37 p.m., Rafael Schloming wrote: > > proton-c/bindings/python/proton/__init__.py, line 2718 > > <https://reviews.apache.org/r/30318/diff/1/?file=836290#file836290line2718> > > > > I like having the ability to control tag generation, but again it is > > odd that this just gets ignored if you happen to pass in bytes. > > > > I think in general send(message) vs send(bytes) should have the > > semantics of simple overloading, in other words the former should be > > equivalento to send(message.encode()). As you have it the semantics are > > actually quite different in these two cases. Agreed, they are quite different. This is deliberate, though I concede not ideal. See response to first comment abive for more detail. - Gordon ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30318/#review69863 ----------------------------------------------------------- On Jan. 27, 2015, 2 p.m., Gordon Sim wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30318/ > ----------------------------------------------------------- > > (Updated Jan. 27, 2015, 2 p.m.) > > > Review request for qpid and Rafael Schloming. > > > Repository: qpid-proton-git > > > Description > ------- > > Sending a message requires several steps: create a delivery with a unique > tag, encode the message, cal send(), call advance. While this gives a lot of > flexibility and power (e.g. streaming messages in chunks without ever holding > the entire thing in memory), for many, many use cases, a simpler solution > would be adequate and would avoid users having to learn what is in my view > one of the more esoteric aspects of the API. > > The attached patch adds send support for a Message object, such that a > message can be sent with a single call. It also allows the send() method to > be 'dual purpose'. It remains backwards compatible as a way of appending > bytes to the current delivery, but can also be used as a one-step call to > send a single message. > > At present the engine examples in python make use of a similar > simplification, but one that is added dynamiccaly and is therefore less clean > and obvious. It currently also doesn't work as well for sending on links > established by the remote peer. This addition would in my view allow for > simpler use where needed, without breaking any existing usage. > > > Diffs > ----- > > proton-c/bindings/python/proton/__init__.py 17cef30 > > Diff: https://reviews.apache.org/r/30318/diff/ > > > Testing > ------- > > > Thanks, > > Gordon Sim > >
