poll-feat by callbackcat · Pull Request #39 · kysect/BotFramework

callbackcat · 2021-08-15T21:41:41Z

Добавлена возможность создавать простые опросы до 10-ти вариантов ответа.
Пока все довольно шатко работает 🥴 некоторые моменты нужно подправить.
Предложения выделяются в "..." и '...'

Added poll feature (raw)

hrrrrustic · 2021-08-15T22:15:57Z

Kysect.BotFramework/ApiProviders/Discord/DiscordApiProvider.cs

+
+        private readonly string[] _emojis =
+        {
+            "", "1️⃣", "2️⃣", "3️⃣","4️⃣","5️⃣","6️⃣","7️⃣", "8️⃣", "9️⃣", "🔟"


@Bibletoon Записывай как работать с эмодзи кекв

(это локальный мем, если что, не обращайте внимания)

hrrrrustic · 2021-08-15T22:16:29Z

Kysect.BotFramework/ApiProviders/Discord/DiscordApiProvider.cs

+                LoggerHolder.Instance.Error("The message wasn't sent by the command " +
+                                            "\"{CommandName}\", the length must not be zero", 
+                                            PollCommand.Descriptor.CommandName);
+                return Result.Ok();


Залогировали ошибку и вернули ок?

hrrrrustic · 2021-08-15T22:17:08Z

Kysect.BotFramework/ApiProviders/Discord/DiscordApiProvider.cs

            var context = new SocketCommandContext(_client, message);
+            if (!context.User.IsBot && context.Message.Content.Contains("!Poll"))
+            {
+                _argsCount = GetPollArguments(message.Content).Count() - 1;


Что за -1?

Я думаю это связано с тем, что на 0 позиции будет название команды, а остальные - это аргументы пула. А он считает именно их кол-во.

Тогда не понял, почему вообще название команды это аргумент команды...

На 0-й позиции будет заголовок опроса. -1 поправлю, глупый момент, согласен

hrrrrustic · 2021-08-15T22:19:45Z

Kysect.BotFramework/ApiProviders/Discord/DiscordApiProvider.cs

+
+            var embed = new EmbedBuilder
+            {
+                Title = arguments[0],


У тебя из метода GetPollArguments может вернуться пустой лист

hrrrrustic · 2021-08-15T22:22:42Z

Kysect.BotFramework/ApiProviders/Discord/DiscordApiProvider.cs

+        private async void ReactWithEmojis(SocketCommandContext context)
+        {
+            for (int i = 1; i < _argsCount; i++)
+            {
+                await context.Message.AddReactionAsync(new Emoji(_emojis[i]))
+                    .ConfigureAwait(false);
+            }
+        }


Да откуда вы все берете ConfigureAwait? Вам кто-то о нем рассказал или что?
async void не используй, без понимания как, почему и зачем просто сделай async Task

hrrrrustic · 2021-08-15T22:24:01Z

Kysect.BotFramework/Core/CommandInvoking/CommandHandler.cs

            }

-            return commandTask.Value.Args.Length == args.Arguments.Count
+            return (commandTask.Value.Args.Count == args.Arguments.Count) || (args.CommandName == "Poll") // Better way to check for Poll?


Что-то вообще ничего не понял

hrrrrustic · 2021-08-15T22:24:31Z

Kysect.BotFramework/Core/Commands/BotCommandDescriptor.cs

-        public string[] Args { get; }
+        public List<string> Args { get; }


В чем суть данного (и пачки ниже) изменения?

Хотел получать переменное кол-во аргументов. Изначально нужно было указывать определенное количество, как я понял. Так же и появилась проблема выше - проверка на полученное количество аргументов и заданное на получение в PollCommand дескрипторе, как args

Вообще ничего не понял
Что значит переменное кол-во аргументов?
Ты хочешь сам изменять аргументы команды? Ты явно делаешь что-то не так

Ну нам нужно получать !Poll + args и непонятно как действовать с string[ ], т.к. он просит указать точное количество args, а их может быть от 1 до 10 по логике, поэтому изменил на лист. Может и правда не так понял 😕

Так тебе приходит запрос уже с каким-то конкретным количеством аргументов, зачем тут лист?
Ну пришло 5 - массив из 5 элементов
7 - из 7
Сам по себе-то массив не меняется

hrrrrustic · 2021-08-15T22:26:01Z

Kysect.BotFramework/DefaultCommands/PollCommand.cs

+        public Task<Result<IBotMessage>> Execute(CommandContainer args)
+        {
+            IBotMessage message = new BotPollMessage(String.Join(" ", args.Arguments));
+            return Task.FromResult(Result.Ok(message));
+        }


Не понял, создал ботмесседж и вернул его
А где сама логика Execute-то?

А она внутри IBotMessage и вызывается BotManager'ом

hrrrrustic · 2021-08-15T22:27:16Z

Kysect.BotFramework/ApiProviders/Discord/DiscordApiProvider.cs

+
+        private List<string> GetPollArguments(string args)
+        {
+            var regex = new Regex(@"[^\s""']+|""([^""]*)""|'([^']*)'"); // Splits into "..." '...' a b c


А в чем смысл создавать каждый раз одну и ту же регулярку? Ну сделай ее статичным полем приватным просто

FrediKats

Очень сильно пострадал DiscordProvider, в него добавилось много логики, которая не принадлежит ему
Ряд замечаний в сторону улучшения объектно-ориентированности кода.

FrediKats · 2021-08-15T22:28:01Z

Kysect.BotFramework/ApiProviders/Discord/DiscordApiProvider.cs

        private readonly object _lock = new object();
        private readonly DiscordSettings _settings;
        private DiscordSocketClient _client;
+        private int _argsCount;


Это не часть логики DiscordApiProvider.

FrediKats · 2021-08-15T22:28:05Z

Kysect.BotFramework/ApiProviders/Discord/DiscordApiProvider.cs

+        private readonly string[] _emojis =
+        {
+            "", "1️⃣", "2️⃣", "3️⃣","4️⃣","5️⃣","6️⃣","7️⃣", "8️⃣", "9️⃣", "🔟"
+        };


Это не часть логики DiscordApiProvider.

FrediKats · 2021-08-15T22:28:40Z

Kysect.BotFramework/ApiProviders/Discord/DiscordApiProvider.cs

                LoggerHolder.Instance.Error("The message wasn't sent by the command " +
-                                            $"\"{PingCommand.Descriptor.CommandName}\", the length must not be zero.");
+                                            "\"{CommandName}\", the length must not be zero", 
+                                            PingCommand.Descriptor.CommandName);


Почему именно PingCommand.Descriptor.CommandName?

А в чем идея этого изменения?

FrediKats · 2021-08-15T22:29:21Z

Kysect.BotFramework/ApiProviders/Discord/DiscordApiProvider.cs

+                LoggerHolder.Instance.Error("The message wasn't sent by the command " +
+                                            "\"{CommandName}\", the length must not be zero", 
+                                            PollCommand.Descriptor.CommandName);
+                return Result.Ok();


А почему тут Result.Ok, если это возвращение при ошибке - не валидном наборе аргументов.

FrediKats · 2021-08-15T22:31:07Z

Kysect.BotFramework/ApiProviders/Discord/DiscordApiProvider.cs

            }

            var context = new SocketCommandContext(_client, message);
+            if (!context.User.IsBot && context.Message.Content.Contains("!Poll"))


Это не должно быть в ApiProvider

Ты хардкодишь конкретную команду тут, так быть не должно иначе этот метод на каждую команду будет получать новый if

А мы разве не поддерживаем кастомные префиксы для команд?

FrediKats · 2021-08-15T22:35:16Z

Kysect.BotFramework/Core/CommandInvoking/CommandHandler.cs

            }

-            return commandTask.Value.Args.Length == args.Arguments.Count
+            return (commandTask.Value.Args.Count == args.Arguments.Count) || (args.CommandName == "Poll") // Better way to check for Poll?


Если есть кастомная логика проверки валидности размера, то предлагаю сделать базовый класс для команд, где будет виртуальный метод IsValidArgumentCount, который проверяет commandTask.Value.Args.Count == args.Arguments.Count, а PollCommand будет наследоваться и оверрайдить со своей кастомной логикой (в идеале там должен быть парсинг и какая-то проверка, а не "если poll - значит точно ок".

FrediKats · 2021-08-15T22:35:50Z

Kysect.BotFramework/DefaultCommands/PollCommand.cs

+            IBotMessage message = new BotPollMessage(String.Join(" ", args.Arguments));
+            return Task.FromResult(Result.Ok(message));


Ничего не понял)
Почему мы на эту команду делаем ничего?

В предыдущих ~~сериях~~ pr'ах была добавлена возможность отправлять не только текст, поэтому команда отвечает только за то, что бы создать сообщение, а вызовом метода отправки занимается BotManager (так как сообщению нужен api provider, что бы отправиться)

FrediKats · 2021-08-15T22:37:21Z

Kysect.BotFramework/ApiProviders/Discord/DiscordApiProvider.cs

+            return options;
+        }
+
+        private async void ReactWithEmojis(SocketCommandContext context)


Мне почему-то кажется, что тут более подходящим названием было AddReactionToMessage.

Имеет смысл вынести логику реакции. Например, сделать класс MessageWithPool, который в конструктор принимал бы message и poolAnswerCount и внутри бы делал вот эту магию с добавлением Emoji.

FrediKats · 2021-08-15T22:41:46Z

Kysect.BotFramework/ApiProviders/Discord/DiscordApiProvider.cs

+        {
+            List<string> arguments = GetPollArguments(text);
+
+            Result<string> result = CheckText(text);


Наверное же сначала нужно делать этот чек, а потом уже GetPoolArgs.

FrediKats · 2021-08-15T22:43:35Z

Kysect.BotFramework/ApiProviders/Discord/DiscordApiProvider.cs

+            for (int i = 1; i < arguments.Count; i++)
+            {
+                arguments[i] = _emojis[i] + "\t" + arguments[i];
+            }
+
+            var embed = new EmbedBuilder
+            {
+                Title = arguments[0],
+                Color = Color.Purple,
+                Description = String.Join("\n", arguments.Skip(1))
+            };
+


Опять же, это выглядит как логика создания какого-то особенного сообщения. Давайте выделим класс PoolInfoMessage, которое будет создаваться от аргументов, внутри вот эти действия делать и ему оставим метод BuildToEmbed.

FrediKats · 2021-08-15T22:47:51Z

Ясно, два шакала одновременно накинулись...

hrrrrustic · 2021-08-15T22:50:47Z

Ясно, два шакала одновременно накинулись...

Буди анчоуса, тройничок устроим

Bibletoon · 2021-08-16T06:05:10Z

Вообще судя по всему, произошёл какое-то небольшое недопонимание задачи. В моём представлении это должно выглядеть примерно так:

Есть какая-то отдельная сущность для опроса, имеющая в себе заголовок опроса и варианты ответов
BotPollMessage как раз таки содержит в себе эту сущность
Пользователь в любой команде может создать опрос и вернуть BotPollMessage

Про варианты ответов в опросе:
Это шаманство с проверкой того, является ли команда опросом (во время проверки количества аргументов) супер лишняя. Во-первых, это костыль. Во-вторых, нам пока не нужно уметь получать переменное количество вариантов ответов от пользователя. То, какие у опроса будут варианты ответов должна решать команда, его вызывающая: там может быть как постоянные варианты (типа да/нет), так и варианты зависящие от переданных аргументов (оставим принятие решения на создателя команды)

Ну и хотелось бы сразу сделать создание опроса в Телеграмме, а не оставлять заглушку (там всё будет проще и ограничится вызовом одного-двух методов библиотеки как мне кажется)

Bibletoon

Если что - пиши, если неудобно писать, можем устроить митинг в дискорде.

FrediKats · 2021-11-05T07:48:38Z

:с

FrediKats · 2022-02-23T12:54:41Z

:сс

poll-feat

3e0cb72

Added poll feature (raw)

callbackcat linked an issue Aug 15, 2021 that may be closed by this pull request

Реализовать возможность отправлять опросы #30

Open

callbackcat requested a review from Bibletoon August 15, 2021 21:42

hrrrrustic reviewed Aug 15, 2021

View reviewed changes

FrediKats suggested changes Aug 15, 2021

View reviewed changes

Bibletoon suggested changes Aug 16, 2021

View reviewed changes

		public string[] Args { get; }
		public List<string> Args { get; }

		IBotMessage message = new BotPollMessage(String.Join(" ", args.Arguments));
		return Task.FromResult(Result.Ok(message));

Conversation

callbackcat commented Aug 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrediKats left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrediKats commented Aug 15, 2021

Uh oh!

hrrrrustic commented Aug 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bibletoon commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bibletoon left a comment

Choose a reason for hiding this comment

Uh oh!

FrediKats commented Nov 5, 2021

Uh oh!

FrediKats commented Feb 23, 2022

hrrrrustic commented Aug 15, 2021 •

edited

Loading

Bibletoon commented Aug 16, 2021 •

edited

Loading