Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
GH-870 Add /msgtoggle command #896
base: master
Are you sure you want to change the base?
GH-870 Add /msgtoggle command #896
Changes from all commits
b5e54ad
ffd29c5
88a71f7
43ea1b0
f054dbd
2437624
aa1ff47
6da3ddf
4a77643
00e97d0
5291625
a091a18
b3dae93
97d5914
76d85d9
df9b84f
4709133
450813c
f22fe08
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trzeba jeszcze przemyśleć trochę to api serwisu - czy jest sens robić 2 osobne serwisy (dla toggle) i czy da się te rzeczy zrobić ładniej bo teraz robi sam się trochę śmietnik w PrivateChatService
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh I don't understand. Please elaborate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe smth more like this:
I think the overall command flow can be simplified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai please explain the difference between my class and vLuckyyy's class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CitralFlo Here are the key differences between the two implementations:
Naming: The suggested version uses more descriptive parameter names (
player
/sender
/target
instead ofcontext
/player
).Code Organization: The suggested version:
setTargetState
methodPotential Issue: There seems to be a logic reversal in the suggested version:
ON
means messages are blocked,OFF
means messages are allowedON
enables messages,OFF
disables messagesThis reversal might cause confusion with the rest of the codebase.
The suggested version offers better code organization, but I'd recommend keeping your toggle logic (ON=blocked, OFF=allowed) to maintain consistency with the existing implementation.
✏️ Learnings added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not nedded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is connected to logic of sending messages. When CommandSender and target are the same. Player receives two messages instead of one. example:
./msgtoggle CitralFlo
sent by CitralFlo will result in two messages.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i mean not nedded as external method, because it's used one time only