[EventHub] only amqp annotated message #19287
Conversation
| :rtype: None | ||
| :raise: :class:`ValueError`, when exceeding the size limit. | ||
| """ | ||
| # pylint:disable=protected-access | ||
| if isinstance(event_data, AmqpAnnotatedMessage): | ||
| event_data = EventData._from_message(event_data._to_outgoing_amqp_message()) # pylint: disable=protected-access |
There was a problem hiding this comment.
is it possible to add the _to_outgoing_message on AmqpAnnotatedMessage like what we have done in Service Bus?
There was a problem hiding this comment.
So I didn't add it because in the SB AmqpAnnotatedMessage _to_outgoing_message, you send in the message arg into ServiceBusMessage. However, EventData does not take keyword args, so we cannot pass in message. If we wanted to do this, we would need to allow for an optional message property on EventData and change the API. Thoughts?
There was a problem hiding this comment.
I think we ultimately would need the kwargs in the constructor.
I'm checking the design guideline
DO craft a constructor for models that are intended to be instantiated by a user (i.e. non-result types) with minimal required information and optional information as keyword-only arguments.
We're missing it in EventData (e.g. properties could be keyword argument in the constructor), but that's a different topic.
I'm good with leaving as it is now as it's an implementation detail, I feel adding kwargs to the constructor while users could not pass anything and there being no documentation is not friendly.
@annatisch , what about your opinions?
|
/azp run python - eventhub - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - eventhub - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - eventhub - tests |
|
No pipelines are associated with this pull request. |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run python - eventhub - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
sdk/eventhub/azure-eventhub/tests/livetest/asynctests/test_send_async.py
Outdated
Show resolved
Hide resolved
sdk/eventhub/azure-eventhub/tests/livetest/asynctests/test_send_async.py
Outdated
Show resolved
Hide resolved
sdk/eventhub/azure-eventhub/tests/livetest/asynctests/test_send_async.py
Outdated
Show resolved
Hide resolved
sdk/eventhub/azure-eventhub/tests/livetest/asynctests/test_send_async.py
Outdated
Show resolved
Hide resolved
sdk/eventhub/azure-eventhub/tests/livetest/asynctests/test_send_async.py
Outdated
Show resolved
Hide resolved
sdk/eventhub/azure-eventhub/tests/livetest/asynctests/test_send_async.py
Outdated
Show resolved
Hide resolved
sdk/eventhub/azure-eventhub/tests/livetest/asynctests/test_send_async.py
Outdated
Show resolved
Hide resolved
|
/azp run python - eventhub - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
...enthub/azure-eventhub/samples/async_samples/send_and_receive_amqp_annotated_message_async.py
Outdated
Show resolved
Hide resolved
sdk/eventhub/azure-eventhub/samples/sync_samples/send_and_receive_amqp_annotated_message.py
Outdated
Show resolved
Hide resolved
sdk/eventhub/azure-eventhub/tests/livetest/asynctests/test_send_async.py
Show resolved
Hide resolved
|
/azp run python - eventhub - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
fixes: #18677