Role command #26
No reviewers
Labels
No Label
Backlog
bug::Awaiting merge
bug::fix
bug::S1
bug::S2
bug::S3
change::Awaiting merge
change::breaking
change::minor
change::new
change::patch
Good First Issue
In Progress
priority::critical
priority::high
priority::low
priority::medium
proposal::accepted
proposal::denied
proposal::pending
No Milestone
No project
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: engineering/community-relations#26
Loading…
Reference in New Issue
There is no content yet.
Delete Branch "master"
Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?
MERGE REQUEST
Description:
New command: role as requested in #6. For some reasons the command isn't loading on ready, and the pipeline failed but the command itself should work.
Status:
You need to add the command to index.ts for it to be properly loaded.
assigned to @matthew
added 1 commit
a475df22- fixCompare with previous version
@Khaazz Requesting review.
changed milestone to %1
That implies that if stop is true at ANY point, you'll end the function there.
However since you used a forEach, you have to use a boolean. You end up iterating over all elements and do the logic for each element.
This is heavy and very much waste of ressource.
Using for of loop would solve this problem easily. Using early return in the for loop would do the job.
However make sure you correctly use
returnorbreak, in order to not make the code less readable. I usually have returns at the beginning of the loop (early return). The only important thing is just that you should see very clearly what is going on when reading the function, not having to wonder why there is a random return somewhere.you can't await a forEach. although it's literally useless in this case since the cb function is not async.
For that matters I would also not use forEach when you have a big chunk of code to run inside the callback function for readability. I would only use forEach when you have a very simple action to perform on each element.
forEach is also slower in general, especially in big arrays.
I already commented in the global try / catch but it should be wrapping the run function in the command class instead.
Logic for general things like argument check (that happens in every commands) that sends back the help command should also probably be moved in the Command class logic.
Although, if you actually want to send a message for each error, then stop, it will behave as expected. But this is imho not such a good idea as it can quickly become spammy.
Additionally, if you really want this behaviour, you should still add / remove the roles that went through in this case.
I still think it's best to go with what I said above. If you want an error message, you can just send one for the case that went wrong. If there are any more errors, they will be detected on the next iteration of the command and a message will be sent accordingly to the user
On a side note, I would personally just do nothing (not try to add / remove) but not errors if the user already has the role that it tries to add / has not the role it tries to remove.
I would only error in case of bad role
I'd prefer a way to have the action and the actual string separated directly.
So you don't have to slice again after. I think it would be more clear.
https://stackoverflow.com/questions/20817618/is-there-a-splice-method-for-strings
same as above regarding forEach. Unless this has changed and i'm not aware of, this won't await nothing.
changed this line in version 3 of the diff
changed this line in version 3 of the diff
added 1 commit
dee8243a- use for of and remove awaitsCompare with previous version
@Hector Was this resolved?
@Hector Was this resolved?
Merged in
6d0499d4Pull request closed