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

Added missing periods at mod-related messages and some system messages #5061

Merged
merged 10 commits into from
Jan 3, 2024

Conversation

fraxxio
Copy link
Contributor

@fraxxio fraxxio commented Jan 1, 2024

Fixes #4515

Changes

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

This should be handled on a per-message basis rather than checked in the SystemMessage constructor

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/messages/MessageBuilder.cpp Outdated Show resolved Hide resolved
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Sorry, I should have been clearer in my initial feedback

The logic should be done where the message is actually created
As an example, the USERNAME has cleared the chat message is created at https://github.com/Chatterino/chatterino2/blob/65b1ed3/src/Application.cpp#L346-L361 so you'd want to just add the period to that specific message instead of inside MessageBuilder at all.
Preferably, when we're done, MessageBuilder.cpp should not have any changes

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Some small nitpicks, otherwise looks good! This is what I meant

src/Application.cpp Outdated Show resolved Hide resolved
src/controllers/commands/builtin/twitch/Announce.cpp Outdated Show resolved Hide resolved
src/controllers/commands/builtin/twitch/DeleteMessages.cpp Outdated Show resolved Hide resolved
src/controllers/commands/builtin/twitch/ShieldMode.cpp Outdated Show resolved Hide resolved
src/providers/irc/AbstractIrcServer.cpp Outdated Show resolved Hide resolved
src/providers/irc/IrcServer.cpp Outdated Show resolved Hide resolved
src/providers/irc/IrcServer.cpp Outdated Show resolved Hide resolved
src/providers/twitch/IrcMessageHandler.cpp Outdated Show resolved Hide resolved
src/controllers/commands/builtin/twitch/Shoutout.cpp Outdated Show resolved Hide resolved
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Lastly, before this can be merged in, pull requests add a changelog entry in the CHANGELOG.md file.
This is a minor change, so it should be added under the ## Unversioned category among the other - Minor: ... entries
A reasonable changelog entry for this change would be: - Minor: Added missing periods at various moderator messages and commands. (#5061), although you're free to reword it a bit as you see fit

src/controllers/commands/builtin/twitch/UpdateColor.cpp Outdated Show resolved Hide resolved
@fraxxio
Copy link
Contributor Author

fraxxio commented Jan 3, 2024

Still missing periods on /untimeout /ban /unban commands responses, not sure where to add those.

fraxxio and others added 4 commits January 3, 2024 11:36
Slightly suboptimal since the . is part of the link now, but this is how
it's done in other places
Slightly suboptimal since the . is part of the link now, but this is how
    it's done in other places
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

I fixed the /ban, /untimeout, and /unban responses now. They did have to be changed in their respective MessageBuilder.cpp code.

Thank you for your contribution!

@pajlada pajlada merged commit 4a0ef08 into Chatterino:master Jan 3, 2024
20 checks passed
@pajlada
Copy link
Member

pajlada commented Jan 3, 2024

Thank you! As a first-time contributor, you can now add yourself to the contributors list that's shown inside the About page in Chatterino.

If you want this, you can open a new PR where you modify the resources/contributors.txt file and add yourself to the list. Make sure to read the comments at the top for instructions.

@fraxxio fraxxio deleted the add-missing-dots-on-mod-messages branch January 6, 2024 18:10
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.

Add missing periods after mod-related messages (e.g. untimeouts)
2 participants