Skip to content

Switch from logging framework Log4J2 to logging facade SLF4J#447

Merged
diemol merged 1 commit intosaucelabs:valfirstlet-users-choose-logging-frameworkfrom
valfirst:let-users-choose-logging-framework
Oct 26, 2023
Merged

Switch from logging framework Log4J2 to logging facade SLF4J#447
diemol merged 1 commit intosaucelabs:valfirstlet-users-choose-logging-frameworkfrom
valfirst:let-users-choose-logging-framework

Conversation

@valfirst
Copy link
Copy Markdown
Contributor

Description

saucerest-java is a client for SauceLabs API (library). In other words it can't be used as a standalone application, it's used as dependency for other libraries and applications. That's why saucerest-java shouldn't have tight coupling with any logging framework and shouldn't have any hardcoded logging configuration (which may bring collisions to users).

Motivation and Context

The problem was introduced in this commit: a989460.
saucerest-java should:

  • rely on logging facade (SLF4J) instead of particular logging framework
  • declare logging configuration only for testing purposes.

How Has This Been Tested?

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (change which improves current code base; please describe the change)
  • [?] Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Screenshots (if appropriate):

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • All new and existing tests passed locally
  • I have added necessary documentation (if appropriate)

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

Copy link
Copy Markdown
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

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

Why don't we remove it completely?

@valfirst
Copy link
Copy Markdown
Contributor Author

Why don't we remove it completely?

Not sure what "it" means here. If the question is about Log4J2, it was moved to be used in unit tests only. If it's needed, I can remove Log4J completely and use some simple provider like slf4j-nop or slf4j-simple

@diemol
Copy link
Copy Markdown
Member

diemol commented Oct 25, 2023

Why don't we remove it completely?

Not sure what "it" means here. If the question is about Log4J2, it was moved to be used in unit tests only. If it's needed, I can remove Log4J completely and use some simple provider like slf4j-nop or slf4j-simple

Yes, that is what I meant. Removing Log4J2 completely.

@valfirst valfirst force-pushed the let-users-choose-logging-framework branch from 2f2955d to 4666bfd Compare October 26, 2023 09:12
@valfirst valfirst force-pushed the let-users-choose-logging-framework branch from 4666bfd to 4b113fa Compare October 26, 2023 09:20
@valfirst valfirst requested a review from diemol October 26, 2023 09:21
Copy link
Copy Markdown
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

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

Thank you, @valfirst!

@diemol diemol changed the base branch from main to valfirstlet-users-choose-logging-framework October 26, 2023 10:22
@diemol diemol merged commit 6eef118 into saucelabs:valfirstlet-users-choose-logging-framework Oct 26, 2023
@valfirst valfirst deleted the let-users-choose-logging-framework branch October 26, 2023 11:49
diemol added a commit that referenced this pull request Oct 26, 2023
…453)

Co-authored-by: Valery Yatsynovich <valery_yatsynovich@epam.com>
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.

2 participants