[Service Bus] Hide encoding and dead_letter_* from Peeked Message #14733
Conversation
rakshith91
requested review from
annatisch,
KieranBrantnerMagee and
yunhaoling
as code owners
ghost
added
the
| @@ -84,7 +84,6 @@ class Message(object): # pylint: disable=too-many-public-methods,too-many-insta | |||
| :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. | |||
There was a problem hiding this comment.
I'm inclining towards just stopping to document the encoding keyword since we just want to hide it for now - thoughts?
| @@ -28,11 +28,11 @@ | |||
|
|
|||
|
|
|||
| class BaseSession(object): | |||
| def __init__(self, session_id, receiver, encoding="UTF-8"): | |||
There was a problem hiding this comment.
would LOVE to change this to keyword only instead
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
rakshith91
changed the title
|
@rakshith91 Can you please resolve the merge conflicts in this PR? I think that is the last thing needed for Kieran and Adam to approve the PR |
rakshith91
force-pushed
the
sb_hideencoding
branch
from
a7bd88d to
28531ad
Compare
| @@ -652,7 +651,7 @@ def dead_letter_reason(self): | |||
| return None | |||
|
|
|||
| @property | |||
| def dead_letter_source(self): | |||
| def _dead_letter_source(self): | |||
There was a problem hiding this comment.
sorry for not pointing out earlier,
these three shall be moved from PeekedMessage to ReceivedMessage as dead_letter_* should be still available on ReceivedMessage.
There was a problem hiding this comment.
@rakshith91 , actually I think you could just revert the change, as my settlement move PR will completely remove the PeekedMessage class #14681
Fixes #14641
Fixes #14640