Skip to content
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

Wiadomość na wzmiankę @everyone #4

Closed
wants to merge 6 commits into from
Closed

Wiadomość na wzmiankę @everyone #4

wants to merge 6 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 11, 2021

No description provided.

main.js Outdated Show resolved Hide resolved
main.js Outdated Show resolved Hide resolved
main.js Outdated Show resolved Hide resolved
main.js Outdated Show resolved Hide resolved
@awaluk awaluk linked an issue Jun 11, 2021 that may be closed by this pull request
main.js Outdated
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..")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Co ma w zasadzie robić ten catch?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Zapomniałem usunąć 😅

main.js Outdated
Comment on lines 40 to 56
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..")
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

main.js Outdated
@@ -30,5 +30,29 @@ client.on('ready', () => {
}
});
});
client.on('message', message => { //Ten event wykonuje się, gdy bot wykryje wiadomość
const pingEmbed = new Discord.MessageEmbed()
.setColor('#eb1540')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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ć.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

main.js Outdated
.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ć.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

main.js Outdated
@@ -30,5 +30,29 @@ client.on('ready', () => {
}
});
});
client.on('message', message => { //Ten event wykonuje się, gdy bot wykryje wiadomość
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.setThumbnail(thumbnail)
.setDescription(description);

const mentionedEveryone = message.content.includes("@everyone") || message.content.includes("@here")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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ą.

Copy link
Member

@ScriptyChris ScriptyChris Jun 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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ć).

.setDescription(description);

const mentionedEveryone = message.content.includes("@everyone") || message.content.includes("@here")
const hasPermission = message.member.hasPermission('MENTION_EVERYONE')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J.w. odnośnie MENTION_EVERYONE

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

@ScriptyChris ScriptyChris Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A co konkretnie się wtedy crashuje?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ScriptyChris ScriptyChris Jun 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

main.js Outdated
@@ -1,3 +1,4 @@
import mentionHandler from './handlers/EveryoneAndHereMentionMessage'
Copy link
Member

@ScriptyChris ScriptyChris Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.setThumbnail(thumbnail)
.setDescription(description);

const mentionedEveryone = message.content.includes("@everyone") || message.content.includes("@here")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

main.js Outdated Show resolved Hide resolved
main.js Outdated
@@ -1,5 +1,6 @@
const Discord = require('discord.js');
const commands = require('./commands');
const EveryoneAndHereMentionMessage = require('./handlers/EveryoneAndHereMentionMessage');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

'@everyone',
'@here'
];
const MentionPermission = 'MENTION_EVERYONE';
Copy link
Member

@ScriptyChris ScriptyChris Jun 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

main.js Outdated
@@ -30,5 +31,7 @@ client.on('ready', () => {
}
});
});

client.on('message', message => {
EveryoneAndHereMentionMessage.mentionHandler(message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 20 to 21
const mentions = message.content.includes(MentionsList[0]) || message.content.includes(MentionsList[1]);
if (mentions) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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ć.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dobra uwaga, naprawię.

Copy link
Member

@d-bend d-bend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jak dla mnie zadowalajace.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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',
});

message.author.send(pingEmbed);
}
} catch(error) {
console.log(error);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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').

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rzucany jest błąd hasPermission is not defined

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


module.exports = {

handler: function (message){
Copy link
Member

@ScriptyChris ScriptyChris Jun 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
});

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

@ScriptyChris ScriptyChris Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 ScriptyChris mentioned this pull request Jul 9, 2021
@ScriptyChris
Copy link
Member

ScriptyChris commented Aug 12, 2021

@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 ghost changed the base branch from master to feature/warn-about-disabled-mentions August 12, 2021 13:17
Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dobra uwaga, naprawię.

message.author.send(pingEmbed);
}
} catch(error) {
console.log(error);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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

Rebase Ci nie działa?

@ghost
Copy link
Author

ghost commented Aug 12, 2021

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

Rebase Ci nie działa?

Tak

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

@awaluk awaluk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funkcjonalnie wszystko wygląda ok, działa

@ScriptyChris
Copy link
Member

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

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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