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

Fix to interpret logger channel names as lowercase #4848

Merged
merged 8 commits into from
Oct 1, 2023

Conversation

GongBingWong
Copy link
Contributor

Description

Fixes the issue (#4835) where logging channels with capitalization wouldn't work, by making channel names lowercase.

I tested the original exe file without my changes and then tested an exe file with my changes and it works.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Felanbird <41973452+Felanbird@users.noreply.github.com>
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/controllers/logging/ChannelLog.cpp Outdated Show resolved Hide resolved
pajlada

This comment was marked as outdated.

@pajlada pajlada linked an issue Sep 30, 2023 that may be closed by this pull request
4 tasks
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

Works as expected, I sent one suggestion as a stylistic thing but otherwise looks good.
Thank you!

src/controllers/logging/ChannelLog.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.

Thanks for your contribution!

In our changelog style, the number behind the changelog entry should be the number of the PR rather than the issue, since we think it gives a bit more context for the whole fix process. Other than that, this works as expected and is the correct solution to this 👍

Only other feedback I have for future contributions is that it's best to use a branch other than master or main or mainline for your PRs, make a specific change branch like fix/logger-lowercase, that way it'll be easier to manage your workflow if you have multiple PRs active to the same repo, and it'll be easier on my end since your branch name doesn't conflict with our main branch name

@pajlada pajlada enabled auto-merge (squash) October 1, 2023 05:19
@pajlada pajlada merged commit ad8f960 into Chatterino:master Oct 1, 2023
17 checks passed
@pajlada
Copy link
Member

pajlada commented Oct 1, 2023

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.

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.

Capitalizing channel names breaks the logger
3 participants