Wiadomość na wzmiankę @everyone · Pull Request #4 · CodersCommunity/discord-info-bot

ghost · 2021-06-11T12:54:41Z

No description provided.

d-bend · 2021-06-11T13:04:12Z

+            if(!message.member.hasPermission('MENTION_EVERYONE')) //Sprawdzamy, czy wysyłający ma uprawnienia do wzmianki @everyone, jeśli nie to bot wysyła wiadomość
+                message.author.send(pingEmbed)
+        }catch{
+            console.log(message.author.username + " wysłał ping w wiadomości prywatnej do bota, hmm..")


Co ma w zasadzie robić ten catch?

Zapomniałem usunąć 😅

d-bend · 2021-06-11T13:10:18Z

+    if (message.content.includes("@everyone")) { //Jeśli wiadomość zawiera @everyone..
+        try{
+            if(!message.member.hasPermission('MENTION_EVERYONE')) //Sprawdzamy, czy wysyłający ma uprawnienia do wzmianki @everyone, jeśli nie to bot wysyła wiadomość
+                message.author.send(pingEmbed)
+        }catch{
+            console.log(message.author.username + " wysłał ping w wiadomości prywatnej do bota, hmm..")
+        }
+
+    } else {
+        if(message.content.includes("@here")) //Jeśli wiadomość zawiera @here..
+        try{
+            if(!message.member.hasPermission('MENTION_EVERYONE')) //Sprawdzamy, czy wysyłający ma uprawnienia do wzmianki @everyone, jeśli nie to bot wysyła wiadomość
+                message.author.send(pingEmbed)
+        }catch{
+            console.log(message.author.username + " wysłał ping w wiadomości prywatnej do bota, hmm..")
+        }
+    }


Zbędna ifologia. Zamiast tego lepsze by było coś w rodzaju:

const mentionedEveryone = (message.content.includes("@everyone") || (message.content.includes("@here") const hasPermission = message.member.hasPermission('MENTION_EVERYONE') if (mentionedEveryone && !hasPermission) { message.author.send(pingEmbed) }

Haha, dzięki wielkie, zapomniałem o "||" w javascriptcie, potem naprawię

ScriptyChris · 2021-06-11T13:39:52Z

 });
+client.on('message', message => { //Ten event wykonuje się, gdy bot wykryje wiadomość
+    const pingEmbed = new Discord.MessageEmbed()
+	.setColor('#eb1540')


Wrzuciłbym wartość koloru do zmiennej/stałej i jej tu użył. Być może w przyszłości będą w różnych miejscach używane jakieś kolory i wtedy przyda się mieć jedno miejsce, gdzie są one zadeklarowane i nazwane, wtedy łatwiej i wygodniej będzie tego używać.

ScriptyChris · 2021-06-11T13:41:03Z

+client.on('message', message => { //Ten event wykonuje się, gdy bot wykryje wiadomość
+    const pingEmbed = new Discord.MessageEmbed()
+	.setColor('#eb1540')
+	.setTitle('Nie wołaj wszystkich!')


Na tłumaczenia jest plik commands.js. A jeśli to tłumaczenie tam kontekstowo nie pasuje, to warto zrobić nowy plik i tam wrzucić tłumaczenia - powód analogiczny jak w poprzednim komentarzu.

ScriptyChris · 2021-06-11T13:41:59Z

+	.setColor('#eb1540')
+	.setTitle('Nie wołaj wszystkich!')
+    .setThumbnail('https://cdn.discordapp.com/attachments/617673807213232128/852887484542615572/Pingsock.png')
+    .setDescription('\nRozumiemy, że potrzebujesz pomocy, ale nie wszyscy chcą zostać o tym powiadomieni.\n Jest nas tutaj dużo i nie ma sensu, aby każdy dostał bezpośrednio taką informację.\n Nie trudno sobie wyobrazić jak irytujące byłoby, gdyby każdy wołał wszystkich do każdego tematu.\n Dlatego zadaj pytanie i po prostu poczekaj - jeśli ktoś będzie wiedział i mógł, to na pewno spróbuje odpowiedzieć.');


J.w. odnośnie tekstu tłumaczenia.

ScriptyChris · 2021-06-11T13:47:35Z

        }
    });
 });
+client.on('message', message => { //Ten event wykonuje się, gdy bot wykryje wiadomość


Tak ogólnie, to przeniósłbym ciało tego event handlera do osobnej nazwanej funkcji, np. handleEveryoneAndHereCommands (która mogła by być importowana z osobnego modułu), bo niewykluczone, że dojdzie w tym handlerze więcej kodu, a wtedy - przy obecnej formie - zrobi się bałagan.

ScriptyChris · 2021-06-11T18:32:40Z

+    .setThumbnail(thumbnail)
+    .setDescription(description);
+
+    const mentionedEveryone = message.content.includes("@everyone") || message.content.includes("@here")


Te stringi @everyone i @here też można wrzucić w zmienne, albo nawet lepiej, zgrupować w jakiś słownik.

Skąd ta funkcja ma dostęp do message? Nie widzę tu podawania referencji w argumentach. Rozumiem że sprawdziłeś to i działa?

Te stringi @everyone i @here też można wrzucić w zmienne, albo nawet lepiej, zgrupować w jakiś słownik.

Myślę, że nie ma takiej potrzeby, gdyż i tak raczej nowe pingi się nie pojawią.

Myślę, że nie ma takiej potrzeby, gdyż i tak raczej nowe pingi się nie pojawią.

Chodzi o to, że te stringi kontekstowo należą do pewnej grupy - odpowiadają za pingowanie userów. Wyobraź sobie sytuację, że funkcjonalność jest rozszerzana i w kilku miejscach korzystasz z tych samych stringów + dochodzą jakieś inne stringi odpowiadające np. komendom (niech będzie /test). W pewnym momencie trudno jest Ci się połapać w kodzie co kontekstowo oznacza dany string, a jakbyś chciał jednak zmienić go w kilku miejscach to musisz te kilka miejsc edytować. W momencie, gdy wrzucisz takie stringi do słownika, to możesz je sobie zgrupować i wtedy szybciej i wygodniej jest się tym posługiwać, np. DICTIONARY.PINGS.EVERYONE i DICTIONARY.PINGS.HERE są łatwiejsze w interpretacji przez programistę niż jakiś randomowy string '@everyone' w kodzie. Poza tym, jeśli np. chcesz sprawdzić w apce jakie podobne stringi są używane, to szukasz pierwszego lepszego (albo idziesz bezpośrednio do miejsca deklaracji słownika) i odnosisz się do jego obiektu, gdzie masz spis wszystkich kontekstowo podobnych stringów - w jednym miejscu (które można na dodatek sobie udokumentować).

ScriptyChris · 2021-06-11T18:33:06Z

+    .setDescription(description);
+
+    const mentionedEveryone = message.content.includes("@everyone") || message.content.includes("@here")
+    const hasPermission = message.member.hasPermission('MENTION_EVERYONE')


J.w. odnośnie MENTION_EVERYONE

ScriptyChris · 2021-06-11T18:34:30Z

+    const mentionedEveryone = message.content.includes("@everyone") || message.content.includes("@here")
+    const hasPermission = message.member.hasPermission('MENTION_EVERYONE')
+    if (mentionedEveryone && !hasPermission) {
+        try{


try..catch jest tu zbędny. To już było wspomniane gdzieś w poprzednim review-komentarzu.

Nie, ponieważ jak nie będzie try, catch, to bot by scrashowal jak ktos napisze @here albo @everyone w prywatnej wiad. do bota

A co konkretnie się wtedy crashuje?

Ok, ale to i tak trzeba obsłużyć. Zobacz jaki błąd w takiej sytuacji wyskakuje, wtedy możnabygo ignorować. Ale inne błędy powinny zostać co najmniej zlogowane albo optymalnie wysłane do jakiegos prawilnego loggera albo sentry.

Sprawdziłem i błąd można zignorować, ponieważ dotyczy on tego, że nie może sprawdzić w prywatnej wiadomości czy ktoś ma permisję. Bez try..catch bot wprost się wyłączy.

nie może sprawdzić w prywatnej wiadomości czy ktoś ma permisję

Czy API Discorda nie pozwala na sprawdzenie czy wiadomość jest prywatna? Można by wtedy zapisać warunek, że bot obsłuży tylko wiadomość publiczną.

Na SO coś o tym piszą i w docsach też coś na ten temat jest:
https://stackoverflow.com/a/48729333/4983840
https://discord.com/developers/docs/resources/channel#channel-object-channel-types

Dobrze, ale to jeden case. Tak jak pisałem wyżej, mogą się pojawiać też inne błędy które tutaj złapiesz ale ich nie obsłużysz. Np. co się stanie jeżeli bot nie będzie mógł wysłać wiadomości na serwer z powodu utraty połączenia/zmiany secretu? Na podstawie tego catcha taka sytuacja będzie nie do odróżnienia od przypadku który opisujesz.

ScriptyChris · 2021-06-11T18:35:59Z

@@ -1,3 +1,4 @@
+import mentionHandler from './handlers/EveryoneAndHereMentionMessage'


Póki co, te moduły korzystają ze składni CommonJS, więc dla spójności warto używać funkcji require zamiast zapisu import z ES6.

d-bend · 2021-06-11T22:20:34Z

+    .setThumbnail(thumbnail)
+    .setDescription(description);
+
+    const mentionedEveryone = message.content.includes("@everyone") || message.content.includes("@here")


Skąd ta funkcja ma dostęp do message? Nie widzę tu podawania referencji w argumentach. Rozumiem że sprawdziłeś to i działa?

ScriptyChris · 2021-06-12T09:53:10Z

@@ -1,5 +1,6 @@
 const Discord = require('discord.js');
 const commands = require('./commands');
+const EveryoneAndHereMentionMessage = require('./handlers/EveryoneAndHereMentionMessage');


Konwencja w JS (i AFAIK nie tylko w nim) jest taka, że z dużej litery nazywa się konstruktory i klasy. Ta funkcja nie jest ani jednym ani drugim, więc powinna być nazwana w formie camelCase everyoneAndHereMentionMessage.

ScriptyChris · 2021-06-12T09:56:58Z

@@ -0,0 +1,30 @@
+const Discord = require('discord.js');
+const MentionsList = [


Tu bardziej niż tablica pasowało by użyć obiektu, ponieważ jest to jakaś mapa słownikowa. Łatwiej wtedy odczytywać wartości z takiego słownika, bo zamiast MentionsList[0] robi się MentionsList.EVERYONE, co nawet IDE może podpowiedzieć. Tablice nadają się do struktur o uporządkowanej kolejności, a tutaj niwelujemy magic stringi do postaci reużywalnych zmiennych.

W TypeScript tu nadał by się enum (który i tak wyjściowo jest obiektem).

ScriptyChris · 2021-06-12T10:02:21Z

+    '@everyone',
+    '@here'
+];
+const MentionPermission = 'MENTION_EVERYONE';


Ponownie co do konwencji nazewniczej - nazwy z dużej litery są używane dla zapisu konstruktorów i klas. Do oznaczenia stałej używa się tzw. SCREAMING_SNAKE_CASE, a więc na przykład: MENTION_PERMISSION. I ta sugestia dotyczy wszystkich stałych w kodzie.

Dzięki za zwrócenie uwagi, naprawię przy kolejnym (czwartym już, eh..) commicie.

ScriptyChris · 2021-06-12T10:06:25Z

 });
-
+client.on('message', message => { 
+    EveryoneAndHereMentionMessage.mentionHandler(message);


Jeśli moduł EveryoneAndHereMentionMessage ma tylko jedną eksportowaną funkcję, to po co ją traktować jako metodę? Prościej użyć po prostu everyoneAndHereMentionMessageHandler - nazwa (ponownie, z małej litery) zawiera w sobie od razu suffix "Handler". Wtedy tę funkcję można wyexportować bezpośrednio z modułu - już bez owijania w obiekt.

d-bend · 2021-06-12T10:19:20Z

+        const mentions = message.content.includes(MentionsList[0]) || message.content.includes(MentionsList[1]);
+        if (mentions) {


adrian17 pisał o tym na discordzie, jest do tego gotowe property w bibliotece z której korzystamy
https://discord.js.org/#/docs/main/stable/class/MessageMentions
Kwestia kosmetyczna ale skoro już jest to warto z tego korzystać.

d u d e, MessageMentions nie wykrywa wzmianki jeśli jest WYŁĄCZONA, a o to chodzi..

d-bend · 2021-06-12T10:25:00Z

+            try{
+                const hasPermission = message.member.hasPermission(MentionPermission);
+                message.author.send(pingEmbed);
+            } catch {


Dalej będę naciskał na tego pustego catcha. Odpowiedziałeś na to tylko częściowo, bo zakładasz jeden przypadek. Co z pozostałymi? Nie uważasz że wypadało chociaż dać console.log na błąd który wystąpił? Jakie inne błędy mogą się tu pojawić? Czy niektóre z nich nie wymagają zatrzymania procesu i zrestartowania bota, tak jak np sytuacja gdy zmienia się token i bot traci uprawnienia do wysyłania wiadomości?

Dobra uwaga, naprawię.

d-bend

Jak dla mnie zadowalajace.

ScriptyChris · 2021-06-12T12:29:02Z

@@ -0,0 +1,30 @@
+const Discord = require('discord.js');
+const MentionsList = {


MentionsList -> mentionsList, albo MENTIONS_LIST (w sumie zamiast "list" bardziej pasuje "dict", bo to nie jest lista - ale to szczegół). Jeśli ten słownik jest traktowany jako stała, to można go też wrzucić w Object.freeze.

const MENTIONS_DICT = Object.freeze({ EVERYONE: '@everyone', HERE: '@here', });

ScriptyChris · 2021-06-12T12:37:25Z

+                message.author.send(pingEmbed);
+            }
+        } catch(error) {
+            console.log(error);


Skoro już jest tu ten try..catch, to dodał bym informację do logowania i użył metody error zamiast log, np. console.error('mentionHandler(..) send error:', error). Ponadto, jeśli oczekiwane jest, że sypnąć może się tylko wywołanie message.author.send, to wyrzuciłbym z bloku try (przeniósł wyżej) tworzenie zmiennej hasPermission (tam się raczej nie ma co wysypać?). Wtedy jeśli się coś sypnie, to będzie można jednoznacznie stwierdzić, że to wysyłanie i informacja w logu na to wskaże. Deklaracja tej zmiennej hasPermission w bloku try - przy założeniu, że ona się nie sypie - IMO trochę zaciemnia przed czym chroni cały try..catch.

Jeśli chodzi o hasPermission w try...catch to jeśli to byłoby poza try...catch to bot, gdy ktoś napisze w wiadomości prywatnej wzmiankę się wysypie

Dlaczego się sypie? Jaki błąd tam jest rzucany? Z tego co kojarzę, to tam chyba jest błąd, że hasPermission is not a function i to by można zaifować albo if (hasPermission) albo if (typeof hasPermission === 'function').

Rzucany jest błąd hasPermission is not defined

No to go zaifuj zamiast robić try..catch.

Problem w tym że discord.js niezbyt pozwala to zifować (albo poprostu jestem zbyt głupi by to zrobić :V)

W jakim sensie nie pozwala?

Zamień:

try{ //needed, if someone sends private message to bot, it crashes without try..catch const hasPermission = message.member.hasPermission(MENTION_PERMISSION);

na:

const hasPermission = message.member.hasPermission && message.member.hasPermission(MENTION_PERMISSION);

, albo:

const hasPermission = typeof message.member.hasPermission === 'function' ? message.member.hasPermission(MENTION_PERMISSION) : false;

Moim zdaniem to nie zadziała, discord.js crashuje się jeśli spróbujesz uzyskać message.member.hasPermission bez try..catch

A próbowałeś? message.member.hasPermission to jest property - jeśli .hasPermission nie jest zdefiniowane, to ono zwraca undefined, ale możesz na nim zrobić typeof, żeby sprawdzić jego typ (to docelowo powinna być funkcja, bo ją wołasz). Nie rozumiem dlaczego takie sprawdzenie miało by nie działać - może pod spodem jest tam jakiś getter lub Proxy, które rzuca błąd przy próbie odczytu wartości niezdefiniowanej, ale na normalnym propertisie powinno się naturalnie móc sprawdzić jego obecność i typ. Jeśli natomiast, w jakimś przypadku, nie ma samego message.member, to wtedy trzeba najpierw sprawdzić jego obecność, a potem dopiero message.member.hasPermission.

ScriptyChris · 2021-06-12T12:44:42Z

+
+module.exports = {
+
+    handler: function (message){


A czemu nie po prostu:

module.exports = function mentionHandler(message) { ... }

albo:

function mentionHandler(message) { ... } module.exports = mentionHandler;

I wtedy użycie:

const mentionHandler = require('./handlers/mentionHandler'); client.on('message', (message) => { mentionHandler(message); });

ScriptyChris · 2021-06-14T18:01:20Z

+            if (mentions && !hasPermission) {
+                message.author.send(pingEmbed);
+            }
+        } catch(error) {


@CodeKid0 upewnij się proszę, czy ten catch łapie błąd, bo wg docsów metoda .send(..) zwraca promisa. Zatem jeśli chce się łapać z niej błędy/wyjątki, to trzeba albo zapiąć się na nią metodą .catch(..) albo użyć na niej await i wtedy strukturalny catch będzie działać.

Moim zdaniem ten catch, w obecnej formie, nie łapie błędu.

ScriptyChris · 2021-08-12T13:16:59Z

@CodeKid0 czy mógłbyś się zrebasować na mój PR? Uporządkowałem tam Twój kod i możesz na nim zrobić fixa, którego zacommitowałeś.

ghost

@CodeKid0 czy mógłbyś się zrebasować na mój PR? Uporządkowałem tam Twój kod i możesz na nim zrobić fixa, którego zacommitowałeś.

Nie mam permisji do twojego PR'a. (Authentication failed)

ghost · 2021-06-12T11:10:52Z

+            try{
+                const hasPermission = message.member.hasPermission(MentionPermission);
+                message.author.send(pingEmbed);
+            } catch {


Dobra uwaga, naprawię.

ghost · 2021-06-12T13:03:25Z

+                message.author.send(pingEmbed);
+            }
+        } catch(error) {
+            console.log(error);


Jeśli chodzi o hasPermission w try...catch to jeśli to byłoby poza try...catch to bot, gdy ktoś napisze w wiadomości prywatnej wzmiankę się wysypie

ScriptyChris · 2021-08-12T13:36:17Z

Nie mam permisji do twojego PR'a. (Authentication failed)

Rebase Ci nie działa?

ghost · 2021-08-12T13:37:32Z

Nie mam permisji do twojego PR'a. (Authentication failed)

Rebase Ci nie działa?

Tak

awaluk

Funkcjonalnie wszystko wygląda ok, działa

ScriptyChris · 2021-09-10T20:02:20Z

Ja nadal chciałbym się dowiedzieć, co z tym try..catch?

Update main.js

2f3bb7e

ghost mentioned this pull request Jun 11, 2021

Odpowiedź na próbę wspomnienia @everyone #3

Open

awaluk reviewed Jun 11, 2021

View reviewed changes

Comment thread main.js Outdated

Comment thread main.js Outdated

Comment thread main.js Outdated

Comment thread main.js Outdated

awaluk linked an issue Jun 11, 2021 that may be closed by this pull request

Odpowiedź na próbę wspomnienia @everyone #3

Open

d-bend reviewed Jun 11, 2021

View reviewed changes

ScriptyChris reviewed Jun 11, 2021

View reviewed changes

Update

11a72d5

ScriptyChris reviewed Jun 11, 2021

View reviewed changes

d-bend requested changes Jun 11, 2021

View reviewed changes

Fix

4a953be

ScriptyChris reviewed Jun 12, 2021

View reviewed changes

d-bend reviewed Jun 12, 2021

View reviewed changes

Poprawiono nazewnictwo, polepszono wyłapywanie błędów

0099923

d-bend approved these changes Jun 12, 2021

View reviewed changes

ScriptyChris reviewed Jun 12, 2021

View reviewed changes

ScriptyChris reviewed Jun 14, 2021

View reviewed changes

ScriptyChris mentioned this pull request Jul 9, 2021

Poprawki do PR-a #4 #7

Open

Fixes from comments (tested)

f42d6e8

ghost changed the base branch from master to feature/warn-about-disabled-mentions August 12, 2021 13:17

ghost commented Aug 12, 2021

View reviewed changes

Rebase #7

93795fc

ghost changed the base branch from feature/warn-about-disabled-mentions to master September 10, 2021 19:51

awaluk approved these changes Sep 10, 2021

View reviewed changes

This pull request was closed.

		@@ -1,3 +1,4 @@
		import mentionHandler from './handlers/EveryoneAndHereMentionMessage'

		@@ -0,0 +1,30 @@
		const Discord = require('discord.js');
		const MentionsList = [

		const mentions = message.content.includes(MentionsList[0]) \|\| message.content.includes(MentionsList[1]);
		if (mentions) {

		@@ -0,0 +1,30 @@
		const Discord = require('discord.js');
		const MentionsList = {

Conversation

ghost commented Jun 11, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

ScriptyChris Jun 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ScriptyChris Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ScriptyChris Jun 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ScriptyChris Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ScriptyChris Jun 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ScriptyChris Jun 12, 2021 •

edited

Loading

ScriptyChris Jun 11, 2021 •

edited

Loading

ScriptyChris Jun 12, 2021 •

edited

Loading

ScriptyChris Jun 11, 2021 •

edited

Loading

ScriptyChris Jun 12, 2021 •

edited

Loading

ScriptyChris Jun 12, 2021 •

edited

Loading