Skip to content

Improve clientmove function #23

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.

Sign up for GitHub

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

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

Improve clientmove function #23

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 22 additions & 4 deletions TS3Connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,15 +352,33 @@ def register_for_unknown_events(self, event_listener=None, weak_ref=True):
if event_listener is not None:
blinker.signal("UNKNOWN").connect(event_listener, weak=weak_ref)

def clientmove(self, channel_id, client_id):
def clientmove(self, channel_id, client_ids, params=None):
"""
Move a client to another channel.
:param channel_id: Channel to move client to.
:param client_id: Id of the client to move.
:param client_ids: List of client Ids or a single client Id to move.
:param params: List of parameters to update in the form param=value.
Copy link
Owner

Choose a reason for hiding this comment

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

I like the addition of params (especially, since clientmove seems to have a channelpw parameter that I haven't implemented). However, I am unsure about this implementation:

  1. I think the ability to additional parameters should be provided for all commands (in case the commands change over time)
  2. I don't particularly like that it is just a string. I would rather work with **kwargs and build the string from that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback. I'll take a look at it asap.

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 had a look at your feedback.

I don't particularly like that it is just a string. I would rather work with **kwargs and build the string from that.

You are talking here about the client_ids, or? Because the params should be already in the respective format.

I'm not really sure, if it's really a good idea to change this here at the moment from my solution to something else. I decided myself for this solution to keep the backward compatibilty for other API users as it was. We could think about changing this overall in a new major version.

What do you think about this?

  1. For now, only change this single function to support params.
  2. Improve overall code with e.g. PyLint and Black (Formatter). I can provide Github workflows for this and adjust the code. This will just take some time as you have a lot of code here, which needs some small adjustments. 😅
  3. Refactor / Improve existing code to support params for all functions (which support params). We could also add it to functions, which do not support params, but this doesn't make sense. Let's rather fail if too many params were given.
  4. Release new major version.

:type channel_id: int
:type client_id: int
:type client_ids: int|list[int]
:type params: list[str]
"""
self._send("clientmove", ["cid=" + str(channel_id), "clid=" + str(client_id)])
if params is None:
params = []

client_id_list = None
if isinstance(client_ids, list):
for client_id in client_ids:
if client_id_list is not None:
client_id_list += "|"

client_id_list += f"clid={str(client_id)}"
else:
client_id_list = f"clid={str(client_ids)}"

params.append(str(client_id_list))
params.append(f"cid={str(channel_id)}")

self._send("clientmove", params)

def clientupdate(self, params=None):
"""
Expand Down