-
Notifications
You must be signed in to change notification settings - Fork 21
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
Extended OnlineClient information variables #43
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As not everyone will want all of this information this should really be an extension not a hard coded set.
Thanks for your reply, I agree. I tried implementing this as an optional string parameter for the |
Added flags for ClientList to use Changed OnlineClient struct to use pointers for optional parameters so it would be clear when no value was returned Added support for client_servergroups which needed comma delimiting. Tried to add/adapt relevant testing for everything changed.
I have added a new commit in which I have tried to address the remarks and questions you made earlier. On the remarks you made above I have added a response with what I wrote this time. Also I have tried editing the *_test.go files to add testing for the standard ClientList and extended Clientlist requests, but I am not that sure if I handled that correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, making good progress.
I have added a new commit trying to implement your recommendations and remarks. Kind regards. |
In the new commit, an embedded struct pointer that holds all optional struct pointers and values for options of clientlist has been added. The |
Hi @stevenh , I hope you are doing well. |
Thanks for the bump @bdeb1337 will try to look over the next few days, but as I no longer have merge access, also poking @lwaddicor |
Hey! Just had a quick look and ran the change against a local teamspeak server I fired up in docker, and it looks as populated as I'd expect. Just a couple of minor things around the comments to please the linters and make it easier for users in their IDEs. Other than that it looks very nice! Validation
Output:
|
* Add comments for forgotten exported types * Add proper comments for the extension consts
Thanks a bunch for checking it out so quickly! I've added your latest suggestions in a new commit. |
Hi @lwaddicor and @stevenh My best wishes for the starting of the new year 🎆 . I would kindly like to bump this PR again as I am looking forward to what you guys think and what I should try to improve or change. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. Looks good to me
No problem, thank you @lwaddicor for checking and merging. And also @stevenh thanks again for the guidance in this PR, appreciate that. |
Hello,
First off thank you for this great library, which is working great for a project of mine.
I was having somewhat the same request as @30 of wanting to expand the basic
clientlist
command. I've read the latest serverquery documentation that ships with ts3 server version 3.13.7 and included all parameters regarding this command:I haven't included
servergroups
yet, since trying to map it as a[]int
didn't immediately work. I guess it would have something to do with helpers.go line 74 which states// TODO(steve): support groups
but I could be wrong. If you are OK with this I could try to give it a go later in a separate pull request? I feel like it might be a little much for one PR otherwise and I could use some guidance in the right direction for that change.Kind regards
Bob