nicklaw5 / helix

A Twitch Helix API client written in Go.
MIT License
240 stars 91 forks source link

Implement Get Chat Settings endpoint #159

Closed pajlada closed 1 year ago

pajlada commented 1 year ago

This PR implements the Get Chat Settings endpoint

Open questions

There are a few optional fields in the response.

1.

These fields are only included if a moderator with the correct scopes is sent along to the moderator_id request parameter

Right now, these fields are not optional in the response struct, so instead of matching the response payload of null, they're set to their default values (so false, empty string, or 0). While this is technically incorrect, using pointers for those 3 values might be less ergonomic so I leave that choice up to you.

I think these fields should be pointers.
Do you think the scope-locked fields should be pointers so users can know whether the fields were included?

2.

These fields are only included if the matching bool field is set to true

These fields are also not pointers, so if follower mode is disabled, follower_mode_duration will be 0 while technically the response we got was null.

I think these should stay pointer-free.
Do you think the fields should be pointers?

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 3664977722


Totals Coverage Status
Change from base Build 3641448789: 0.06%
Covered Lines: 1374
Relevant Lines: 1477

💛 - Coveralls
nicklaw5 commented 1 year ago

Do you think the scope-locked fields should be pointers so users can know whether the fields were included? Do you think the fields should be pointers?

We haven't used pointers for data structs throughout this library. I'm not sure what implications that will have for un/marshaling JSON objects. Possibly none, but it will need to be tested.

Does Twitch support fields with null values? There may be some validation on their end that we need to consider.

pajlada commented 1 year ago

This specific endpoint returns a literal null for the slow_mode_wait_time, follower_mode_duration, and potentially non_moderator_chat_delay_duration values in case their respective settings are disabled.

Non-moderator response with all settings off Fields `moderator_id`, `non_moderator_chat_delay`, and `non_moderator_chat_delay_duration` absent ```json { "data": [ { "broadcaster_id": "11148817", "slow_mode": false, "slow_mode_wait_time": null, "follower_mode": false, "follower_mode_duration": null, "subscriber_mode": false, "emote_mode": false, "unique_chat_mode": false } ] } ```
Moderator response with all settings off and no chat delay All fields present ```json { "data": [ { "broadcaster_id": "11148817", "moderator_id": "11148817", "slow_mode": false, "slow_mode_wait_time": null, "follower_mode": false, "follower_mode_duration": null, "subscriber_mode": false, "emote_mode": false, "unique_chat_mode": false, "non_moderator_chat_delay": false, "non_moderator_chat_delay_duration": null } ] } ```
Moderator response with slow mode set to 5s ```json { "data": [ { "broadcaster_id": "11148817", "moderator_id": "11148817", "slow_mode": true, "slow_mode_wait_time": 5, "follower_mode": false, "follower_mode_duration": null, "subscriber_mode": false, "emote_mode": false, "unique_chat_mode": false, "non_moderator_chat_delay": false, "non_moderator_chat_delay_duration": null } ] } ```
Moderator response with follower mode and the non-moderator chat delay setting enabled ```json { "data": [ { "broadcaster_id": "22484632", "moderator_id": "11148817", "slow_mode": false, "slow_mode_wait_time": null, "follower_mode": true, "follower_mode_duration": 10080, "subscriber_mode": false, "emote_mode": false, "unique_chat_mode": false, "non_moderator_chat_delay": true, "non_moderator_chat_delay_duration": 1 } ] } ```
pajlada commented 1 year ago

And if the library has never used pointers, this is probably not the right place to start doing so, so I change my suggestion to not have any of the fields as pointers.

If a need arises to know whether the fields were there, it would be as a check for "does this user access token have the correct scope & moderator access in this channel", at which point we could add a new field for that shield that users could use alongside the Data field

nicklaw5 commented 1 year ago

Sounds good. Did you need to make any other changes before I merge?

pajlada commented 1 year ago

Nope, I'm happy with this as is. Thank you!

joeyak commented 1 year ago

Wasn't sure the place to do it, but this works with the context of the ticket I guess. @nicklaw5

We haven't used pointers for data structs throughout this library. I'm not sure what implications that will have for un/marshaling JSON objects. Possibly none, but it will need to be tested.

The way the json packages handles nulls is if there's no data, then it's null, if there is then fill it. I had to do that where my database has some optional objects that could go in, and so there wouldn't be circular references, I made some objects pointers.

Here's a snippet showing it https://go.dev/play/p/p0vUWPiwk85