MovingBlocks / Terasology

Terasology - open source voxel world
http://terasology.org
Apache License 2.0
3.64k stars 1.32k forks source link

qa: address PMD GuardLogStatement warnings #5209

Closed soloturn closed 1 month ago

soloturn commented 5 months ago

using fluent logger avoids evaluating methods, and fixes the PMD warning: log not surrounded by "if". this is in case the argument to a parameterized logger is a method call, like here:

- logger.error("onAudioEnd() notification failed for {}", entry.getValue(), e);
+ logger.atError().addArgument(() -> entry.getValue()).addArgument(e).log("onAudioEnd() notification failed for {}");

Edit by niruandaleth/jdrueckert: PMD warnings were fixed in different ways, among others the following variant of the fluent API which is less verbose than the one originally proposed (see above):

- logger.error("onAudioEnd() notification failed for {}", entry.getValue(), e);
+ logger.atError().log("onAudioEnd() notification failed for {}", entry.getValue(), e);
BenjaminAmos commented 4 months ago

This might take a while to review (92 files changed) but at a cursory glance there's a few questions that occurred to me about this:

soloturn commented 4 months ago
  • For the PMD warning (log not surrounded by "if"), is this about still evaluating methods even when logging is disabled? Is there a significant impact already from not doing this?

true. methods still would be called. depending which method it is the impact varies. not calling the method when not logging has, contrary, a clear and predictable impact: no code executed. which is good.

  • The "fluent" logger style appears more verbose and makes logging harder to distinguish from existing code. Is this to be used everywhere or just where a method is called for a log parameter?

i changed it only where a method is called as log parameter and pmd warned.

  • How would you suggest we enforce this change? I suspect that new code will bring new log statements and they are highly likely to use the original logger methods unless enforced.

we could turn on pmd again, it warns or blocks depending how we set it. there is no problem to use original logger methods, at least imo. there are other opinionated approaches, like using google flogger, which would convert everything, but that might be too much for terasology. personally i would not enforce it, but rely on peoples working brains on code review. as well as local gradle clean build warning about it. but that is not my call, i am happy with any approach you guys take.

soloturn commented 4 months ago

This might take a while to review (92 files changed)

you find it easier to chunk it?