Support configure log level for recipe changes #852
Conversation
| @Parameter(property = "rewrite.recipeChangeLogLevel", defaultValue = "WARN") | ||
| protected LogLevel recipeChangeLogLevel; | ||
|
|
||
| protected void log(CharSequence content) { |
There was a problem hiding this comment.
All fine with this change. Just wanted to explain why I implemented it the way I originally did.
Now you essentially have a method in an abstract class, available in all the different implementations, and it only communicates "log" and a string to be logged. It is very implicit that the actual log level is affected by a LogLevel field with very specific semantics. I didn't want to introduce a method named only "log", which is intended for a very specific use: to log something that changed due to a rewrite.
To me, it seems better to have a method which accepts an explicit log level (e.g. the configured recipeChangeLogLevel), because the knowledge about what is logged is at the site where one invokes the logging-method. Now one really has to know that the method named only "log" is specifically for logging changes made from a recipe, instead of the method accepting the loglevel as a parameter and carrying out as instructed, and the knowledge about why a certain loglevel is used is kept at the call site.
This can be implemented as a method accepting the LogLevel in the abstract class, absolutely, and that would also align with my original "design". Externalizing it to a separate (package scoped) class is just my preference to not tie too much concerns into abstract classes and making them available through inheritance.
Just wanted to mention it for you to consider :)
There was a problem hiding this comment.
A (better IMHO) alternative would be to perhaps have
protected void log(LogLevel level, CharSequence content) {instead, as it would not tie the log-method implicitly to the recipeChangeLogLevel. And you avoid the separate class.
It would be very easy for other code to evolve to invoke the new log-method accepting only the message, and not logging anything related to "changes from rewriting", but gets the log level unintentionally configured from recipeChangeLogLevel.
There was a problem hiding this comment.
If you prefer not repeating (passing) recipeChangeLogLevel from the different logging sites, I would rename the logging-method specifically for its intended use, perhaps logRecipeChange(CharSequence) or something, to communicate its intended use. It is not a general log method.
There was a problem hiding this comment.
Thanks for that context! I've adopted your proposal of log(LogLevel, CharSequence)
What's changed?
Adding configurable log level for when running recipe(s) result in changes:
The default is "WARN" to align with existing behaviour.
It only affects logging from recipe results (which are currently hardcoded to
.warn(..)). Any other logging is intentionally not affected by this property.What's your motivation?
I, and I see indications from others as well, use OpenRewrite to always rewrite existing code from some source. I.e. the rewriting and changes are expected and handled by further steps in a build. For these use cases it is not appropriate to log performed changes as warnings. The motivation is to keep a clean build log, and only issue warnings for actual issues.
Example use-cases I believe are relevant for this change:
build-helper-maven-plugin#755,Anything in particular you'd like reviewers to focus on?
The name of the configuration property. I currently named is as
recipeChangeLogLevel, with the intention to communicate that it controls the log level used for logging changes performed by recipes. I am very open for any better name, if anyone has an idea for it 😅Checklist
I am one of those people coding in Eclipse, but I believe the small changes introduced here should not be crashing with how IntelliJ would format the source files.