-
Notifications
You must be signed in to change notification settings - Fork 17
Implement channel name update rate limiting in Bot class #78
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
base: dev
Are you sure you want to change the base?
Conversation
|
Also added a warning as per #77 |
|
I appreciate the pull request! I'll review this when I get home from holiday. We might want to break this time limit stuff into its own interface and class though, but I'm highly for having this implemented into the bot. |
|
I wrote it quite quickly as i wasnt sure if there was a desire for this functionality. happy to break it out :) |
| bot.Information.Id.ToString()); | ||
| } | ||
| } | ||
| } |
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.
I believe it's best to do this warning message whenever we are running IConfigurable#Configure, as this is where we are populating and starting the bots.
PlayerCountDiscordBot/DiscordPlayerCountBot/Configuration/DockerConfiguration.cs
Lines 40 to 67 in 6cd7df7
| foreach (var botName in botNames) | |
| { | |
| var address = botAddresses.ElementAtOrDefault(index); | |
| var port = botPorts.ElementAtOrDefault(index); | |
| var token = botTokens.ElementAtOrDefault(index) ?? throw new ApplicationException("Missing bot token."); | |
| var status = botStatuses.ElementAtOrDefault(index); | |
| var statusFormat = statusFormats.ElementAtOrDefault(index); | |
| var nameAsLabel = botTags.ElementAtOrDefault(index); | |
| var channelID = channelIds.ElementAtOrDefault(index); | |
| var provider = EnumHelper.GetDataProvider(providerTypes.ElementAtOrDefault(index)); | |
| var info = new BotInformation() | |
| { | |
| Name = botName, | |
| Address = string.Format("{0}:{1}", address, port), | |
| Token = token, | |
| Status = status, | |
| StatusFormat = statusFormat, | |
| UseNameAsLabel = nameAsLabel, | |
| ChannelID = channelID, | |
| ProviderType = provider | |
| }; | |
| var bot = new Bot.Bot(info, applicationTokens, dataProviders); | |
| await bot.StartAsync(shouldStart); | |
| bots.Add(bot.Information.Id.ToString(), bot); | |
| index++; | |
| } |
PlayerCountDiscordBot/DiscordPlayerCountBot/Configuration/StandardConfiguration.cs
Lines 43 to 48 in 6cd7df7
| foreach (var info in config.ServerInformation) | |
| { | |
| var bot = new Bot.Bot(info, config.ApplicationTokens, dataProviders); | |
| await bot.StartAsync(shouldStart); | |
| bots.Add(bot.Information!.Id.ToString(), bot); | |
| } |
I believe the message is clear on what is going on; however, I do not like to concat strings like this and would rather use a string builder instead.
https://learn.microsoft.com/en-us/dotnet/api/system.text.stringbuilder?view=net-9.0
|
|
||
| await DiscordClient.SetGameAsync(LastKnownStatus, null, (ActivityType)activityInteger); | ||
| await DiscordClient.SetChannelName(Information.ChannelID, LastKnownStatus); | ||
|
|
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.
I believe that there is an interface and classes to be built here to support this. I think that we would want some sort of generic solution just in case Discord ever imposes a rate limit on any other part of the application.
public interface IRateLimiter
{
/// <summary>
/// Determines if the operation can be executed under current rate limits.
/// </summary>
bool CanExecute(string key);
/// <summary>
/// Records an operation attempt.
/// </summary>
void RecordExecution(string key);
}The above is the interface that I think full encompasses what a rate limit would be.
As far as what is the implementation of what would use these objects I cannot say at this moment, I can only image in the short term, for the usage would be:
await obj.TryExecuteAsync("rename_channel", async () =>
{
Console.WriteLine("Renaming channel..."); // Could do anything, not just log.
});Maybe the key shouldn't be a string and should be an enum?
What are your thoughts?
|
yep will work towards the above |
This change allows people to specify an update value of less than 300 seconds, when having channel name updates enabled.
I found an issue that I wanted the bots to be as up to date as possible. But had to specify an Update Time greater than 300 seconds otherwise channel names went a bit crazy.
Added a rather rudimentary check that validates whether then channel name update occurred greater than 5 minutes ago.