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

Separate message names from logging levels #909

merged 3 commits into from Apr 30, 2018


Copy link

@Pr0methean Pr0methean commented Apr 29, 2018

This removes ".warn.", ".error." and ".info." from the names of logging messages in, for several reasons:

  1. Console messages about the same subject matter now appear together, even when they are split across multiple logging levels.
  2. Calls to the LoggableLocalizedStringImpl constructor already specify the logging level, so embedding it in the name violates the Don't Repeat Yourself and Separation of Concerns principles.
  3. If we ever need to change the logging level of a message once multiple locales exist, we then have to update it in all versions of the resource file, and we make translators more susceptible to merge conflicts.
  4. We may eventually want to use the same message in multiple situations, at different logging levels.

This PR does not change the hierarchy inside LocalizedStrings, for two reasons:

  1. The current scheme makes it clear what level we're logging at, without having to jump from the calling class to LocalizedStrings. Thus, while it still violates DRY and SoC, in that case it may be a price worth paying for the readability gain.
  2. If we do start using the same message at multiple logging levels, then each level will need a different LoggableLocalizedString instance, and we'll need to disambiguate between them. The current scheme accomplishes that.
@Pr0methean Pr0methean requested a review from smartboyathome Apr 29, 2018
@Pr0methean Pr0methean requested a review from aramperes Apr 29, 2018
@aramperes aramperes merged commit 9bfcea8 into GlowstoneMC:dev Apr 30, 2018
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
license/cla Contributor License Agreement is signed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants