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

Payload option for Channel.join() function #136

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Maturin
Copy link

@Maturin Maturin commented May 6, 2024

What kind of change does this PR introduce?

Implement the #134 feature request.

What is the current behavior?

#134

What is the new behavior?

When calling the join function, one can specify an additional payload respectively parameters.

Additional context

n/a

@Maturin Maturin requested a review from a team as a code owner May 6, 2024 07:59
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @Maturin - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 3 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@@ -36,23 +36,27 @@ def __init__(self, socket: Socket, topic: str, params: Dict[str, Any] = {}) -> N
self.listeners: List[CallbackListener] = []
self.joined = False

def join(self) -> Channel:
def join(self, payload: Dict[str, Any] = {}) -> Channel:
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (code_clarification): Consider documenting the default behavior when 'payload' is not provided.

It might be helpful for users of this method to understand what the default payload behavior entails, especially if it affects the channel's state or interaction with the server.

return self

async def _join(self) -> None:
async def _join(self, payload: Dict[str, Any]) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (code_refinement): Parameter 'payload' in '_join' should be optional to match the public 'join' method.

Making 'payload' optional in '_join' ensures that calls without a payload do not fail and maintains consistency with the public interface.

Suggested change
async def _join(self, payload: Dict[str, Any]) -> None:
async def _join(self, payload: Optional[Dict[str, Any]] = None) -> None:

Comment on lines +43 to +44
:param payload: Optional, additional payload, which is sent to the
server, when a channel is joined.
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (code_clarification): Clarify the impact of the 'payload' on the server's response or channel state.

Expanding on how the payload affects the server's handling of the join request or the state of the channel could provide clearer guidance to developers using this method.

Suggested change
:param payload: Optional, additional payload, which is sent to the
server, when a channel is joined.
:param payload: Optional, additional payload, which is sent to the
server when a channel is joined. This payload can be used
to pass extra data or preferences that might influence how
the server processes the join request or manages the channel's
state. For example, it could specify initial settings or
roles for the user in the channel.

@Maturin
Copy link
Author

Maturin commented May 6, 2024

This pull requested needs to be merged with PR #135.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant