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

#404: logging concept #760

Draft
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

KianRolf
Copy link
Contributor

draft pr for: #404

Attempt to create custom slf4j provider by implementing 3 slf4j interfaces. Will result in the following error
Screenshot 2024-11-13 161610

@jan-vcapgemini jan-vcapgemini changed the title Enhance/404 logging concept #404: logging concept Nov 15, 2024
@coveralls
Copy link
Collaborator

coveralls commented Nov 18, 2024

Pull Request Test Coverage Report for Build 12933945130

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 22 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.1%) to 68.299%

Files with Coverage Reduction New Missed Lines %
com/devonfw/tools/ide/log/IdeSubLoggerOut.java 5 26.47%
com/devonfw/tools/ide/cli/Ideasy.java 17 56.52%
Totals Coverage Status
Change from base Build 12927675457: -0.1%
Covered Lines: 7231
Relevant Lines: 10206

💛 - Coveralls

@hohwille
Copy link
Member

hohwille commented Nov 28, 2024

This PR is still in New state so I never looked at it before.
Most probably Jan-V. wanted to do a team review and probably complete things together with Kian.
Anyway, here is my Review feedback:

  • Great that you found this trick with META-INF/services to control which implementation SLF4J will use. This is the concept used by Java ServiceLoader. A lot of Java developers with years of development experience have never heard of such things before so it is great that you figured this out :)
  • You properly found a way to provide our own logger implementation for SLF4J 👍
  • Please add JavaDoc: Your methods are already annotated with @Override - that is great so nothing is to be done here but your classes should have a JavaDoc:
/**
 * Implementation of {@Logger} for this project. Allows logging to the console with coloured output.
 */
public class TestLogger implements Logger {

BTW: you should avoid fully-qualified types in your code and use import statements instead (only use fully-qualified types when necessary because you are using 2 different types with the same simple name).

  • Further, you should rename this class: It is located in src/main/java and that is correct, because it is to be used for the productive usage by the end-user. The name TestLogger implies that is will only be used for testing. A better name could be e.g. IdeLoggerAdapter. or IdeSlf4jLoggerImpl.
  • Finally, your logger implementation is more or less empty. I guess your are fully aware of that but since the PR is not in draft state, I just wanted to explicitly mention this because here comes the tricky part: You need to delegate to IdeLogger and do additional things. Therefore I assume this PR cannot be finished by you and someone would need to take over.

@KianRolf KianRolf marked this pull request as draft November 28, 2024 15:58
jan-vcapgemini and others added 2 commits January 21, 2025 11:35
jan-vcapgemini and others added 3 commits January 22, 2025 10:48
renamed TestLogger to IdeLoggerAdapter
added missing param to javadoc
renamed TestProvider to TestProviderImpl
renamed TestLoggerFactory to TestLoggerFactoryImpl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Team Review
Development

Successfully merging this pull request may close these issues.

4 participants