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

refactor(root): move logging settings/configuration into a single package #220

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

notjuanortiz
Copy link
Contributor

This PR includes a few minor, but related, changes.

  • Moves LoggingSettings, and LoggingConfigurationFactory into a single package io.luna.logging
  • Separates file reading logic from logging settings/configuration (see Luna.class), This was done in case someone wants to change the file type being read (currently .toml).
  • Includes unit test for TomlLoggingSettingFileReader to ensure the code works as intended.

Contributes to #215 .

@TravisBuddy
Copy link

Hey @notjuanortiz,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 879c0450-6d33-11ea-8920-b3945d724fa9

@notjuanortiz notjuanortiz changed the title refactor: move logging settings/configuration into a single package refactor(root): move logging settings/configuration into a single package Mar 25, 2020
@stale
Copy link

stale bot commented May 24, 2020

This issue has been marked as stale because it has not had any recent activity. It will be closed if no further activity occurs.

@stale stale bot added the wontfix An issue that went stale or won't be resolved. label May 24, 2020
@stale
Copy link

stale bot commented May 31, 2020

This issue has been closed because it has not had any recent activity. Feel free to re-open this issue or create another one if the problem isn't resolved.

@stale stale bot closed this May 31, 2020
@lare96 lare96 removed the wontfix An issue that went stale or won't be resolved. label Dec 28, 2020
@lare96 lare96 reopened this Dec 28, 2020
Copy link
Member

@lare96 lare96 left a comment

Choose a reason for hiding this comment

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

Looks good, but I think the old TOML loading was more clear and concise. Could it be changed back?

@notjuanortiz
Copy link
Contributor Author

826565c

By creating an abstraction I've decoupled reading the logging settings from a file from actually starting the server.
I also made support for reading settings from other file types much easier to implement. I'm not sure I understand what the motivation to revert this change is.

@notjuanortiz notjuanortiz requested a review from lare96 June 30, 2021 17:00
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.

3 participants