Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Service Bus] Hide encoding and dead_letter_* from Peeked Message #14733

Merged
merged 7 commits into from
Oct 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sdk/servicebus/azure-servicebus/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ now raise more concrete exception other than `MessageSettleFailed` and `ServiceB
* Exceptions `MessageSendFailed`, `MessageSettleFailed` and `MessageLockExpired`
now inherit from `azure.servicebus.exceptions.MessageError`.
* `get_state` in `ServiceBusSession` now returns `bytes` instead of a `string`.
* `encoding` support is removed from `ServiceBusMessage`

**BugFixes**

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ class ServiceBusMessage(object): # pylint: disable=too-many-public-methods,too-
:keyword str to: The `to` address used for auto_forward chaining scenarios.
:keyword str reply_to: The address of an entity to send replies to.
:keyword str reply_to_session_id: The session identifier augmenting the `reply_to` address.
:keyword str encoding: The encoding for string data. Default is UTF-8.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclining towards just stopping to document the encoding keyword since we just want to hide it for now - thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to me

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seconded.


:ivar AMQPMessage amqp_message: Advanced use only. The internal AMQP message payload that is sent or received.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ def __init__(
)

self._populate_attributes(**kwargs)
self._session = ServiceBusSession(self._session_id, self, self._config.encoding) if self._session_id else None
self._session = ServiceBusSession(self._session_id, self) if self._session_id else None

def __iter__(self):
return self._iter_contextual_wrapper()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@


class BaseSession(object):
def __init__(self, session_id, receiver, encoding="UTF-8"):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would LOVE to change this to keyword only instead

Copy link
Contributor

@yunhaoling yunhaoling Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong preference here, either works for me as session is something that user shouldn't create by themselves, and we don't document it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined not to because of what Adam said. Kwarg hiding is workable for public surface areas, but still suboptimal there, and in a way that I'm nervous to expose us to internally: namely, you don't notice typod arguments any more.

Let me know if this isn't a convincing argument and I can try harder, don't want to come in and flip the table :P

# type: (str, Union[ServiceBusReceiver, ServiceBusReceiverAsync], str) -> None
def __init__(self, session_id, receiver):
# type: (str, Union[ServiceBusReceiver, ServiceBusReceiverAsync]) -> None
self._session_id = session_id
self._receiver = receiver
self._encoding = encoding
self._encoding = "UTF-8"
self._session_start = None
self._locked_until_utc = None # type: Optional[datetime.datetime]
self.auto_renew_error = None
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ def __init__(
)

self._populate_attributes(**kwargs)
self._session = ServiceBusSession(self._session_id, self, self._config.encoding) if self._session_id else None
self._session = ServiceBusSession(self._session_id, self) if self._session_id else None

# Python 3.5 does not allow for yielding from a coroutine, so instead of the try-finally functional wrapper
# trick to restore the timeout, let's use a wrapper class to maintain the override that may be specified.
Expand Down