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

fix: Use self-defined logging level instead of org.slf4j.event.Level #3796

Merged

Conversation

slarse
Copy link
Collaborator

@slarse slarse commented Feb 15, 2021

#3775

This PR introduces a self-defined LogLevel enum that can be used to get or set the logging level. The reason we need this is that org.slf4j.event.Level makes Spoon backwards-incompatible with libraries that uses slf4j-api < 1.7.15. Which is not good. I've tested this locally and it fixes the test errors in Casper, as mentioned in #3775.

This is a breaking change, as the return type of Environment.getLevel() is changed. #I purposefully changed the name of the type from Level to LogLevel to make this more evident. #3755 was in fact also breaking, as the org.slf4j.event.Level is not the same as the previous Level type from log4j. With this change, however, we won't have breakage again, as we now define the type ourselves.

I have two more thoughts on this.

  1. Do we want to implement this breaking change now, or should we wait for Spoon 9? If we don't want to do it now, we must also revert refactor: use slf4j API instead of log4j-api #3755.
  2. As we're breaking the API anyway, we may also want to consider renaming Environment.get/setLevel() to Environment.get/setLogLevel(). These names are more descriptive ("level" is obvious in a logging framework, but not in a metaprogramming library), and also make the breaking change more obvious.

@slarse slarse changed the title wip: fix: Use self-defined logging level instead of org.slf4j.event.Level review: fix: Use self-defined logging level instead of org.slf4j.event.Level Feb 16, 2021
@slarse
Copy link
Collaborator Author

slarse commented Feb 18, 2021

@monperrus per our discussion, changed LogLevel to Level.

@monperrus
Copy link
Collaborator

Perfect, LGTM, will merge.

@monperrus monperrus changed the title review: fix: Use self-defined logging level instead of org.slf4j.event.Level fix: Use self-defined logging level instead of org.slf4j.event.Level Feb 22, 2021
@monperrus monperrus merged commit 900a9a6 into INRIA:master Feb 22, 2021
@monperrus
Copy link
Collaborator

Very important follow-up of #3775, thanks very much.

@slarse slarse deleted the issue/3775-use-self-defined-logging-level branch May 28, 2021 10:08
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