Merge pull request #38 from NoiTheCat/cleanup

Code cleanup

Removes various unneeded things from code and rewrites the shard status display.

ApplicationCommands/BirthdayModule.cs

@@ -132,7 +132,7 @@ public class BirthdayModule : BotModuleBase {
         var query = GetSortedUserList(Context.Guild);
 
         // TODO pagination instead of this workaround
-        bool hasOutputOneLine = false;
+        var hasOutputOneLine = false;
         // First output is shown as an interaction response, followed then as regular channel messages
         async Task doOutput(string msg) {
             if (!hasOutputOneLine) {
@@ -146,8 +146,7 @@ public class BirthdayModule : BotModuleBase {
         var output = new StringBuilder();
         var resultCount = 0;
         output.AppendLine("Recent and upcoming birthdays:");
-        for (int count = 0; count <= 21; count++) // cover 21 days total (7 prior, current day, 14 upcoming)
+        for (var count = 0; count <= 21; count++) { // cover 21 days total (7 prior, current day, 14 upcoming)
-        {
             var results = from item in query
                           where item.DateIndex == search
                           select item;

ApplicationCommands/BirthdayOverrideModule.cs

@@ -3,7 +3,6 @@ using Discord.Interactions;
 using static BirthdayBot.Common;
 
 namespace BirthdayBot.ApplicationCommands;
-
 [RequireBotModerator]
 [Group("override", HelpCmdOverride)]
 public class BirthdayOverrideModule : BotModuleBase {

ApplicationCommands/BotModuleBase.cs

@@ -39,7 +39,7 @@ public abstract class BotModuleBase : InteractionModuleBase<SocketInteractionCon
     /// throwing a FormatException if the input is not recognized.
     /// </summary>
     protected static string ParseTimeZone(string tzinput) {
-        if (!TzNameMap.TryGetValue(tzinput, out string? tz))
+        if (!TzNameMap.TryGetValue(tzinput, out var tz))
             throw new FormatException(":x: Unknown time zone name.\n" +
                 "To find your time zone, please refer to: https://kevinnovak.github.io/Time-Zone-Picker/");
         return tz!;

ApplicationCommands/ConfigModule.cs

@@ -97,8 +97,8 @@ public class ConfigModule : BotModuleBase {
 
         internal static async Task CmdSetMessageResponse(SocketModal modal, SocketGuildChannel channel,
                                                          Dictionary<string, SocketMessageComponentData> data) {
-            string? newSingle = data[ModalComCidSingle].Value;
+            var newSingle = data[ModalComCidSingle].Value;
-            string? newMulti = data[ModalComCidMulti].Value;
+            var newMulti = data[ModalComCidMulti].Value;
             if (string.IsNullOrWhiteSpace(newSingle)) newSingle = null;
             if (string.IsNullOrWhiteSpace(newMulti)) newMulti = null;
 
@@ -156,7 +156,7 @@ public class ConfigModule : BotModuleBase {
             var existing = db.BlocklistEntries
                 .Where(bl => bl.GuildId == user.Guild.Id && bl.UserId == user.Id).FirstOrDefault();
 
-            bool already = (existing != null) == setting;
+            var already = (existing != null) == setting;
             if (already) {
                 await RespondAsync($":white_check_mark: User is already {(setting ? "" : "not ")}blocked.").ConfigureAwait(false);
                 return;
@@ -171,13 +171,13 @@ public class ConfigModule : BotModuleBase {
 
         [SlashCommand("set-moderated", HelpPfxModOnly + "Set moderated mode on the server.")]
         public async Task CmdSetModerated([Summary(name: "enable", description: "The moderated mode setting.")] bool setting) {
-            bool current = false;
+            var current = false;
             await DoDatabaseUpdate(Context, s => {
                 current = s.Moderated;
                 s.Moderated = setting;
             });
 
-            bool already = setting == current;
+            var already = setting == current;
             if (already) {
                 await RespondAsync($":white_check_mark: Moderated mode is already **{(setting ? "en" : "dis")}abled**.");
             } else {
@@ -204,7 +204,7 @@ public class ConfigModule : BotModuleBase {
         result.AppendLine($"Server time zone: `{ (guildconf.GuildTimeZone ?? "Not set - using UTC") }`");
         result.AppendLine();
 
-        bool hasMembers = Common.HasMostMembersDownloaded(guild);
+        var hasMembers = Common.HasMostMembersDownloaded(guild);
         result.Append(DoTestFor("Bot has obtained the user list", () => hasMembers));
         result.AppendLine($" - Has `{guild.DownloadedMemberCount}` of `{guild.MemberCount}` members.");
         int bdayCount = default;
@@ -238,7 +238,7 @@ public class ConfigModule : BotModuleBase {
             announcech = guild.GetTextChannel((ulong)(guildconf.AnnouncementChannel ?? 0));
             return announcech != null;
         }));
-        string disp = announcech == null ? "announcement channel" : $"<#{announcech.Id}>";
+        var disp = announcech == null ? "announcement channel" : $"<#{announcech.Id}>";
         result.AppendLine(DoTestFor($"(Optional) Bot can send messages into { disp }", delegate {
             if (announcech == null) return false;
             return guild.CurrentUser.GetPermissions(announcech).SendMessages;
@@ -251,7 +251,7 @@ public class ConfigModule : BotModuleBase {
 
         const int announceMsgPreviewLimit = 350;
         static string prepareAnnouncePreview(string announce) {
-            string trunc = announce.Length > announceMsgPreviewLimit ? announce[..announceMsgPreviewLimit] + "`(...)`" : announce;
+            var trunc = announce.Length > announceMsgPreviewLimit ? announce[..announceMsgPreviewLimit] + "`(...)`" : announce;
             var result = new StringBuilder();
             foreach (var line in trunc.Split('\n'))
                 result.AppendLine($"> {line}");

ApplicationCommands/HelpModule.cs

@@ -1,7 +1,6 @@
 using Discord.Interactions;
 
 namespace BirthdayBot.ApplicationCommands;
-
 public class HelpModule : BotModuleBase {
     private const string TopMessage =
         "Thank you for using Birthday Bot!\n" +
@@ -33,12 +32,10 @@ public class HelpModule : BotModuleBase {
     public async Task CmdHelp() {
         const string DMWarn = "Please note that this bot works in servers only. " +
             "The bot will not respond to any other commands within a DM.";
-
-        string ver =
 #if DEBUG
-            "DEBUG flag set";
+        var ver = "DEBUG flag set";
 #else
-            "v" + System.Reflection.Assembly.GetExecutingAssembly().GetName().Version!.ToString(3);
+        var ver = "v" + System.Reflection.Assembly.GetExecutingAssembly().GetName().Version!.ToString(3);
 #endif
         var result = new EmbedBuilder()
             .WithAuthor("Help & About")
@@ -48,6 +45,6 @@ public class HelpModule : BotModuleBase {
             .AddField("Commands", RegularCommandsField)
             .AddField("Moderator commands", ModCommandsField)
             .Build();
-        await RespondAsync(text: (Context.Channel is IDMChannel ? DMWarn : null), embed: result).ConfigureAwait(false);
+        await RespondAsync(text: Context.Channel is IDMChannel ? DMWarn : null, embed: result).ConfigureAwait(false);
     }
 }

ApplicationCommands/ModalResponder.cs

@@ -1,5 +1,4 @@
 namespace BirthdayBot.ApplicationCommands;
-
 /// <summary>
 /// An instance-less class meant to handle incoming submitted modals.
 /// </summary>
@@ -28,7 +27,6 @@ static class ModalResponder {
             await handler(arg, channel, data).ConfigureAwait(false);
         } catch (Exception e) {
             inst.Log(nameof(ModalResponder), $"Unhandled exception. {e}");
-            // TODO when implementing proper application error logging, see here
             await arg.RespondAsync(ShardInstance.InternalError);
         }
     }

ApplicationCommands/Preconditions/EnforceBlocking.cs

@@ -2,7 +2,6 @@
 using Discord.Interactions;
 
 namespace BirthdayBot.ApplicationCommands;
-
 /// <summary>
 /// Only users not on the blocklist or affected by moderator mode may use the command.<br/>
 /// This is used in the <see cref="BotModuleBase"/> base class. Manually using it anywhere else is unnecessary.

ApplicationCommands/Preconditions/RequireGuildContext.cs

@@ -1,7 +1,6 @@
 using Discord.Interactions;
 
 namespace BirthdayBot.ApplicationCommands;
-
 /// <summary>
 /// Implements the included precondition from Discord.Net, requiring a guild context while using our custom error message.<br/><br/>
 /// Combining this with <see cref="RequireBotModeratorAttribute"/> is redundant. If possible, only use the latter instead.

BackgroundServices/BackgroundService.cs

@@ -1,5 +1,4 @@
 namespace BirthdayBot.BackgroundServices;
-
 abstract class BackgroundService {
     protected static SemaphoreSlim DbConcurrentOperationsLock { get; } = new(ShardManager.MaxConcurrentOperations);
     protected ShardInstance ShardInstance { get; }

BackgroundServices/BirthdayRoleUpdate.cs

@@ -111,7 +111,7 @@ class BirthdayRoleUpdate : BackgroundService {
     private static async Task<IEnumerable<SocketGuildUser>> UpdateGuildBirthdayRoles(SocketGuild g, SocketRole r, HashSet<ulong> toApply) {
         var additions = new List<SocketGuildUser>();
         try {
-            var removals = new List<SocketGuildUser>(); // TODO check if roles can be removed in-place instead of building a list first
+            var removals = new List<SocketGuildUser>();
             var no_ops = new HashSet<ulong>();
 
             // Scan role for members no longer needing it

BackgroundServices/ExternalStatisticsReporting.cs

@@ -1,7 +1,6 @@
 using System.Text;
 
 namespace BirthdayBot.BackgroundServices;
-
 /// <summary>
 /// Reports user count statistics to external services on a shard by shard basis.
 /// </summary>

Common.cs

@@ -1,7 +1,6 @@
 using System.Text;
 
 namespace BirthdayBot;
-
 static class Common {
     /// <summary>
     /// Formats a user's name to a consistent, readable format which makes use of their nickname.
@@ -42,7 +41,7 @@ static class Common {
         if (guild.MemberCount > 30) {
             // For guilds of size over 30, require 85% or more of the members to be known
             // (26/30, 42/50, 255/300, etc)
-            int threshold = (int)(guild.MemberCount * 0.85);
+            var threshold = (int)(guild.MemberCount * 0.85);
             return guild.DownloadedMemberCount >= threshold;
         } else {
             // For smaller guilds, fail if two or more members are missing

Configuration.cs

@@ -6,7 +6,6 @@ using System.Reflection;
 using System.Text.RegularExpressions;
 
 namespace BirthdayBot;
-
 /// <summary>
 /// Loads and holds configuration values.
 /// </summary>
@@ -15,7 +14,6 @@ class Configuration {
 
     public string BotToken { get; }
     public string? DBotsToken { get; }
-    public bool QuitOnFails { get; }
 
     public int ShardStart { get; }
     public int ShardAmount { get; }
@@ -47,7 +45,6 @@ class Configuration {
 
         BotToken = ReadConfKey<string>(jc, nameof(BotToken), true);
         DBotsToken = ReadConfKey<string>(jc, nameof(DBotsToken), false);
-        QuitOnFails = ReadConfKey<bool?>(jc, nameof(QuitOnFails), false) ?? false;
 
         ShardTotal = args.ShardTotal ?? ReadConfKey<int?>(jc, nameof(ShardTotal), false) ?? 1;
         if (ShardTotal < 1) throw new Exception($"'{nameof(ShardTotal)}' must be a positive integer.");

Data/BotDatabaseContext.cs

@@ -2,7 +2,6 @@
 using Npgsql;
 
 namespace BirthdayBot.Data;
-
 public class BotDatabaseContext : DbContext {
     private static readonly string _connectionString;
 

Data/Extensions.cs

@@ -1,5 +1,4 @@
 namespace BirthdayBot.Data;
-
 internal static class Extensions {
     /// <summary>
     /// Gets the corresponding <see cref="GuildConfig"/> for this guild, or a new one if one does not exist.

Data/Migrations/.editorconfig

@@ -0,0 +1,4 @@
+[*.cs]
+generated_code = true
+dotnet_analyzer_diagnostic.category-CodeQuality.severity = none
+dotnet_diagnostic.CS1591.severity = none

Data/UserEntry.cs

@@ -2,7 +2,6 @@
 using System.ComponentModel.DataAnnotations.Schema;
 
 namespace BirthdayBot.Data;
-
 [Table("user_birthdays")]
 public class UserEntry {
     [Key]

GlobalSuppressions.cs

@@ -1,9 +0,0 @@
-// This file is used by Code Analysis to maintain SuppressMessage
-// attributes that are applied to this project.
-// Project-level suppressions either have no target or are given
-// a specific target and scoped to a namespace, type, member, etc.
-
-using System.Diagnostics.CodeAnalysis;
-
-[assembly: SuppressMessage("Style", "IDE0161:Convert to file-scoped namespace",
-    Scope = "namespace", Target = "~N:BirthdayBot.Data.Migrations")]

Program.cs

@@ -43,13 +43,14 @@ class Program {
     public static void ProgramStop() {
         if (_stopping) return;
         _stopping = true;
-        Log("Shutdown", "Commencing shutdown...");
+        Log(nameof(Program), "Shutting down...");
 
         var dispose = Task.Run(_bot!.Dispose);
-        if (!dispose.Wait(90000)) {
+        if (!dispose.Wait(30000)) {
-            Log("Shutdown", "Normal shutdown has not concluded after 90 seconds. Will force quit.");
+            Log(nameof(Program), "Disconnection is taking too long. Will force exit.");
             Environment.ExitCode &= (int)ExitCodes.ForcedExit;
         }
+        Log(nameof(Program), $"Uptime: {BotUptime}");
         Environment.Exit(Environment.ExitCode);
     }
 

Properties/PublishProfiles/FolderProfile.pubxml

@@ -1,14 +0,0 @@
-<?xml version="1.0" encoding="utf-8"?>
-<!--
-https://go.microsoft.com/fwlink/?LinkID=208121. 
--->
-<Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
-  <PropertyGroup>
-    <PublishProtocol>FileSystem</PublishProtocol>
-    <Configuration>Release</Configuration>
-    <Platform>Any CPU</Platform>
-    <TargetFramework>net6.0</TargetFramework>
-    <PublishDir>bin\Release\net6.0\publish\</PublishDir>
-    <SelfContained>false</SelfContained>
-  </PropertyGroup>
-</Project>

ShardInstance.cs

@@ -46,7 +46,6 @@ public sealed class ShardInstance : IDisposable {
 
         // Background task constructor begins background processing immediately.
         _background = new ShardBackgroundWorker(this);
-        Log(nameof(ShardInstance), "Instance created.");
     }
 
     /// <summary>
@@ -66,7 +65,6 @@ public sealed class ShardInstance : IDisposable {
         DiscordClient.LogoutAsync().Wait(5000);
         DiscordClient.Dispose();
         _interactionService.Dispose();
-        Log(nameof(ShardInstance), "Instance disposed.");
     }
 
     internal void Log(string source, string message) => Program.Log($"Shard {ShardId:00}] [{source}", message);
@@ -127,7 +125,6 @@ public sealed class ShardInstance : IDisposable {
             await _interactionService.ExecuteCommandAsync(context, _services).ConfigureAwait(false);
         } catch (Exception e) {
             Log(nameof(DiscordClient_InteractionCreated), $"Unhandled exception. {e}");
-            // TODO when implementing proper application error logging, see here
             if (arg.Type == InteractionType.ApplicationCommand) {
                 if (arg.HasResponded) await arg.ModifyOriginalResponseAsync(prop => prop.Content = InternalError);
                 else await arg.RespondAsync(InternalError);
@@ -161,7 +158,6 @@ public sealed class ShardInstance : IDisposable {
                 await context.Interaction.RespondAsync(errReply, ephemeral: true).ConfigureAwait(false);
             } else {
                 // Generic error response
-                // TODO when implementing proper application error logging, see here
                 var ia = context.Interaction;
                 if (ia.HasResponded) await ia.ModifyOriginalResponseAsync(p => p.Content = InternalError).ConfigureAwait(false);
                 else await ia.RespondAsync(InternalError).ConfigureAwait(false);

ShardManager.cs

@@ -13,12 +13,7 @@ class ShardManager : IDisposable {
     /// <summary>
     /// Number of seconds between each time the status task runs, in seconds.
     /// </summary>
-    private const int StatusInterval = 60;
+    private const int StatusInterval = 90;
-
-    /// <summary>
-    /// Number of shards allowed to be destroyed before the program may close itself, if configured.
-    /// </summary>
-    private const int MaxDestroyedShards = 10; // TODO make configurable
 
     /// <summary>
     /// Number of concurrent shard startups to happen on each check.
@@ -28,8 +23,7 @@ class ShardManager : IDisposable {
 
     /// <summary>
     /// Amount of time without a completed background service run before a shard instance
-    /// is considered "dead" and tasked to be removed. A fraction of this value is also used
+    /// is considered "dead" and tasked to be removed.
-    /// to determine when a shard is "slow".
     /// </summary>
     private static readonly TimeSpan DeadShardThreshold = new(0, 20, 0);
 
@@ -43,7 +37,6 @@ class ShardManager : IDisposable {
 
     private readonly Task _statusTask;
     private readonly CancellationTokenSource _mainCancel;
-    private int _destroyedShards = 0;
 
     internal Configuration Config { get; }
 
@@ -62,7 +55,7 @@ class ShardManager : IDisposable {
         // Start status reporting thread
         _mainCancel = new CancellationTokenSource();
         _statusTask = Task.Factory.StartNew(StatusLoop, _mainCancel.Token,
-                                              TaskCreationOptions.LongRunning, TaskScheduler.Default);
+                                            TaskCreationOptions.LongRunning, TaskScheduler.Default);
     }
 
     public void Dispose() {
@@ -90,8 +83,6 @@ class ShardManager : IDisposable {
     /// Creates and sets up a new shard instance.
     /// </summary>
     private async Task<ShardInstance> InitializeShard(int shardId) {
-        ShardInstance newInstance;
-
         var clientConf = new DiscordSocketConfig() {
             ShardId = shardId,
             TotalShards = Config.ShardTotal,
@@ -106,8 +97,8 @@ class ShardManager : IDisposable {
             .AddSingleton(s => new DiscordSocketClient(clientConf))
             .AddSingleton(s => new InteractionService(s.GetRequiredService<DiscordSocketClient>()))
             .BuildServiceProvider();
-        newInstance = services.GetRequiredService<ShardInstance>();
+        var newInstance = services.GetRequiredService<ShardInstance>();
-        await newInstance.StartAsync().ConfigureAwait(false);
+        await newInstance.StartAsync();
 
         return newInstance;
     }
@@ -120,105 +111,62 @@ class ShardManager : IDisposable {
         return null;
     }
 
-    #region Status checking and display
-    private struct GuildStatusData {
-        public int GuildCount;
-        public TimeSpan LastTaskRunTime;
-        public string? ExecutingTask;
-    }
-
-    private static string StatusDisplay(IEnumerable<int> guildList, Dictionary<int, GuildStatusData> guildInfo, bool showDetail) {
-        if (!guildList.Any()) return "--";
-        var result = new StringBuilder();
-        foreach (var item in guildList) {
-            result.Append(item.ToString("00") + " ");
-            if (showDetail) {
-                result.Remove(result.Length - 1, 1);
-                result.Append($"[{Math.Floor(guildInfo[item].LastTaskRunTime.TotalSeconds):000}s");
-                if (guildInfo[item].ExecutingTask != null)
-                    result.Append($" {guildInfo[item].ExecutingTask}");
-                result.Append("] ");
-            }
-        }
-        if (result.Length > 0) result.Remove(result.Length - 1, 1);
-        return result.ToString();
-    }
-
     private async Task StatusLoop() {
         try {
             while (!_mainCancel.IsCancellationRequested) {
-                Log($"Bot uptime: {Program.BotUptime}");
+                Log($"Uptime: {Program.BotUptime}");
 
-                // Iterate through shard list, extract data
+                // Iterate through shards, create report on each
-                var guildInfo = new Dictionary<int, GuildStatusData>();
+                var shardStatuses = new StringBuilder();
-                var now = DateTimeOffset.UtcNow;
                 var nullShards = new List<int>();
-                foreach (var item in _shards) {
+                var deadShards = new List<int>();
-                    if (item.Value == null) {
+                for (var i = 0; i < _shards.Count; i++) {
-                        nullShards.Add(item.Key);
+                    shardStatuses.Append($"Shard {i:00}: ");
+
+                    if (_shards[i] == null) {
+                        shardStatuses.AppendLine("Inactive.");
+                        nullShards.Add(i);
                         continue;
                     }
-                    var shard = item.Value;
-
-                    guildInfo[item.Key] = new GuildStatusData {
-                        GuildCount = shard.DiscordClient.Guilds.Count,
-                        LastTaskRunTime = now - shard.LastBackgroundRun,
-                        ExecutingTask = shard.CurrentExecutingService
-                    };
-                }
-
-                // Process info
-                var guildCounts = guildInfo.Select(i => i.Value.GuildCount);
-                var guildTotal = guildCounts.Sum();
-                var guildAverage = guildCounts.Any() ? guildCounts.Average() : 0;
-                Log($"Currently in {guildTotal} guilds. Average shard load: {guildAverage:0.0}.");
-
-                // Health report
-                var goodShards = new List<int>();
-                var badShards = new List<int>(); // shards with low connection score OR long time since last work
-                var deadShards = new List<int>(); // shards to destroy and reinitialize
-                foreach (var item in guildInfo) {
-                    var lastRun = item.Value.LastTaskRunTime;
 
+                    var shard = _shards[i]!;
+                    var client = shard.DiscordClient;
+                    shardStatuses.Append($"{Enum.GetName(typeof(ConnectionState), client.ConnectionState)} ({client.Latency:000}ms).");
+                    shardStatuses.Append($" Guilds: {client.Guilds.Count}.");
+                    shardStatuses.Append($" Background: {shard.CurrentExecutingService ?? "Idle"}");
+                    var lastRun = DateTimeOffset.UtcNow - shard.LastBackgroundRun;
                     if (lastRun > DeadShardThreshold / 3) {
-                        badShards.Add(item.Key);
+                        // Formerly known as a 'slow' shard
-
+                        shardStatuses.Append($", heartbeat {Math.Floor(lastRun.TotalMinutes):00}m ago.");
-                        // Consider a shard dead after a long span without background activity
-                        if (lastRun > DeadShardThreshold)
-                            deadShards.Add(item.Key);
                     } else {
-                        goodShards.Add(item.Key);
+                        shardStatuses.Append('.');
+                    }
+                    
+                    shardStatuses.AppendLine();
+
+                    if (lastRun > DeadShardThreshold) {
+                        shardStatuses.AppendLine($"Shard {i:00} marked for disposal.");
+                        deadShards.Add(i);
                     }
                 }
-                Log("Online: " + StatusDisplay(goodShards, guildInfo, false));
+                Log(shardStatuses.ToString().TrimEnd());
-                if (badShards.Count > 0) Log("Slow: " + StatusDisplay(badShards, guildInfo, true));
-                if (deadShards.Count > 0) Log("Dead: " + StatusDisplay(deadShards, guildInfo, false));
-                if (nullShards.Count > 0) Log("Offline: " + StatusDisplay(nullShards, guildInfo, false));
 
                 // Remove dead shards
                 foreach (var dead in deadShards) {
                     _shards[dead]!.Dispose();
                     _shards[dead] = null;
-                    _destroyedShards++;
-                }
-                if (Config.QuitOnFails && _destroyedShards > MaxDestroyedShards) {
-                    Environment.ExitCode = (int)Program.ExitCodes.DeadShardThreshold;
-                    Program.ProgramStop();
-                } else {
-                    // Start up any missing shards
-                    var startAllowance = MaxConcurrentOperations;
-                    foreach (var id in nullShards) {
-                        // To avoid possible issues with resources strained over so many shards starting at once,
-                        // initialization is spread out by only starting a few at a time.
-                        if (startAllowance-- > 0) {
-                            _shards[id] = await InitializeShard(id).ConfigureAwait(false);
-                        } else break;
-                    }
                 }
 
-                await Task.Delay(StatusInterval * 1000, _mainCancel.Token).ConfigureAwait(false);
+                // Start null shards, a few at at time
+                var startAllowance = MaxConcurrentOperations;
+                foreach (var id in nullShards) {
+                    if (startAllowance-- > 0) {
+                        _shards[id] = await InitializeShard(id);
+                    } else break;
+                }
+
+                await Task.Delay(StatusInterval * 1000, _mainCancel.Token);
             }
         } catch (TaskCanceledException) { }
     }
-    #endregion
 }