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

Remove logback dependency #155

Merged
merged 4 commits into from
Oct 5, 2021
Merged

Remove logback dependency #155

merged 4 commits into from
Oct 5, 2021

Conversation

sandeepmattepu
Copy link
Contributor

Let the users of Java-OCA-OCPP choose which logging to use.

sandeepmattepu and others added 2 commits September 30, 2021 15:11
Let the users of Java-OCA-OCPP choose which logging to use.
@sandeepmattepu
Copy link
Contributor Author

Updated the versions in pom.xml files

@TVolden
Copy link
Member

TVolden commented Oct 4, 2021

Hi @sandeepmattepu ,

Thanks for your pull request.

I don't know much about logback, but I can see the logback was included in #39 by @eupakhomov. I don't know if he has any comments?

Am I right in assuming that removing the logback does not break anything, since the logging is done via the wrapper interface?

Sincerely,
Thomas Volden

@sandeepmattepu
Copy link
Contributor Author

Hi Thomas,

All the logging in your code base is done using slf4j. slf4j is logging interface and it allows users to plug in whatever logging framework they want to use at deployment time.

Please see the "Files changed" tab, because I have made the scope of logback dependency to test for ocpp-common (pom.xml).

When I have packaged the project (i.e mvn package), I have noticed two similar warnings for OCPP-J and ocpp-v1_6 tests :

> SLF4J: Failed to load class "org.slf4j.impl.StaticLoggerBinder".
> SLF4J: Defaulting to no-operation (NOP) logger implementation

I have fixed this by adding logback to both OCPP-J and ocpp-v1_6 with scope as test.

Now mvn package has compiled, all the tests ran with no problems. Yes removing logback should not break anything.

Best regards
Sandeep Mattepu

@TVolden TVolden merged commit f2410da into ChargeTimeEU:master Oct 5, 2021
@TVolden
Copy link
Member

TVolden commented Oct 5, 2021

Thanks for your contribution @sandeepmattepu.
I have merged the changes, and will try to publish the changes tomorrow.

@sandeepmattepu sandeepmattepu deleted the patch-1 branch October 5, 2021 20:30
@eupakhomov
Copy link
Contributor

Sorry for being late to the party :)
slf4j + logback were introduced as a replacement for log4j for the reasons mentioned in #39.
While it might be a good idea to let the users decide which logging they want to use, I would suggest to mention in the README that they need to add the corresponding dependency to not to have them surprised with disappeared logs in case they relied on the libs logging (which maybe not the most common but still possible case).

@sandeepmattepu sandeepmattepu mentioned this pull request Oct 8, 2021
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