From b3edb2c2292a319604bbfed0cd89764f3ecd404d Mon Sep 17 00:00:00 2001 From: jdrueckert Date: Sun, 10 Mar 2024 21:00:51 +0100 Subject: [PATCH 1/3] chore: remove year from idea copyright template (#5221) --- .idea/copyright/Terasology_Foundation.xml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.idea/copyright/Terasology_Foundation.xml b/.idea/copyright/Terasology_Foundation.xml index e71f8fdb9df..a9157f2a0dc 100644 --- a/.idea/copyright/Terasology_Foundation.xml +++ b/.idea/copyright/Terasology_Foundation.xml @@ -2,7 +2,7 @@ - \ No newline at end of file + From bcae39f6cdd82f4eac6695fe54e456a1e12de1dc Mon Sep 17 00:00:00 2001 From: jdrueckert Date: Sun, 10 Mar 2024 21:30:03 +0100 Subject: [PATCH 2/3] doc: guidance on PMD guard log statement warning (#5220) * doc: guidance on PMD guard log statement warning * chore: address review comments * chore: more detailed note on verbose fluent logging API variant * chore: apply review suggestions --- docs/Code-Conventions.md | 96 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 94 insertions(+), 2 deletions(-) diff --git a/docs/Code-Conventions.md b/docs/Code-Conventions.md index ba2146fe1db..4d9e960a78d 100644 --- a/docs/Code-Conventions.md +++ b/docs/Code-Conventions.md @@ -6,6 +6,7 @@ Note that we try and stick to the standard Java conventions as much as possible. - [Starred Imports](#starred-imports) - [Others](#others) - [Checkstyle](#checkstyle) +- [PMD](#pmd) - [Testing](#testing) - [JavaDoc](#javadoc) @@ -112,8 +113,8 @@ Of note is that IntelliJ is known to not behave nicely when it comes to disablin ## Checkstyle We use the [Checkstyle](http://checkstyle.sourceforge.net/) project to help keep the codebase consistent with some code conventions. -The Terasology engine and all modules use the same configuration as defined in [MovingBlocks/TeraConfig](https://github.com/movingblocks/teraconfig). -You can find the local copy of the configuration file at `config/metrics/checkstyle/checkstyle.xml`. +The Terasology engine and all modules use the same configuration as defined in [MovingBlocks/TeraConfig](https://github.com/MovingBlocks/TeraConfig). +You can find the local copy of the configuration file at [`config/metrics/checkstyle/checkstyle.xml`](https://github.com/MovingBlocks/TeraConfig/blob/master/checkstyle/checkstyle.xml). When working an area please keep an eye out for existing warnings to fix in files you're impacting anyway. Make sure to run Checkstyle on any new files you add since that's the best time to fix warnings. @@ -139,6 +140,97 @@ The Checkstyle configuration file is located at `config/metrics/checkstyle/check Checkstyle statistics are automatically calculated per build and turned into nifty metrics per build. For instance, have a look at the [Checkstyle Report for the engine](http://jenkins.terasology.io/teraorg/job/Terasology/job/engine/job/develop/checkstyle). +## PMD + +We use the [PMD](https://pmd.github.io/) project to help keep the codebase free from common programming flaws like unused variables, empty catch blocks, unnecessary object creation, etc. +The Terasology engine's configuration is defined in [MovingBlocks/TeraConfig](https://github.com/MovingBlocks/TeraConfig). +You can find the local copy of the configuration file at [`config/metrics/pmd/pmd.xml`](https://github.com/MovingBlocks/TeraConfig/blob/master/pmd/pmd.xml). + +When working an area please keep an eye out for existing warnings to fix in files you're impacting anyway. +Make sure to run PMD on any new files you add since that's the best time to fix warnings. + +### Check Execution + +#### Gradle + +You can easily run PMD via the Gradle wrapper `gradlew`. +See [PMD Gradle Plugin](https://docs.gradle.org/current/userguide/pmd_plugin.html) for more information. + +- `gradlew check` to run all checks and tests (incl. PMD) +- `gradlew engine:pmdMain` to run PMD for just the engine's main source directory + +#### IntelliJ Integration + +We recommend to use the [PMD IDEA Plugin](https://plugins.jetbrains.com/plugin/15412-pmd-idea). +IntelliJ IDEA allows to easily run PMD checks for the whole project, specific folders, or just single files. + +In case the IDE does not pick put the correct PMD rules automatically you may have to configure it manually. +The PMD configuration file is located at `config/metrics/checkstyle/pmd.xml`. + +### Guard Log Statement Warnings + +The [PMD GuardLogStatement rule](https://pmd.github.io/pmd/pmd_rules_java_bestpractices.html#guardlogstatement) identifies logs that are not guarded resulting in the String creation and manipulation as well as any associated calculations be performed even if the respective log level or the entire logging support are disabled. + +See the following example for a non-guarded log statement: +```java +logger.debug("log something" + method() + " and " + param.toString() + "concat strings"); +``` + +#### Parameter Substitution + +In general, parameter substitution is a sufficient log guard. +It also removes the need for explicit `toString()` calls. +Parameter substitution can be applied to the above-mentioned non-guarded log statement as follows: +```java +logger.debug("log something {} and {}", method(), param); +``` + +Unfortunately, PMD is currently subject to a [bug](https://github.com/pmd/pmd/issues/4703) that can lead to warnings still being reported despite the parametrized logging approach. +If you utilized parameter substitution but are still facing the `GuardLogStatement` PMD warning, alternative approaches need to be found until the bug is resolved. +These include local variable creation, suppression, and the fluent log API. +For a decision about the appropriate approach, the context, the log level, and the individual performance impact of any relevant case should be taken into consideration. + +#### Local Variable Creation + +If the calculation performed was already performed before or will be performed after the log line (including in further log lines), introducing a local variable and referencing it in the log is a suitable approach. +In some cases a respective local variable may already exist and simply be referenced in your log statement. +```java +String localVar = method(); +logger.debug("log something {} and {}", localVar, param); +[...] +logger.debug("log something else {}", localVar); +``` + +#### Suppression + +For Terasology complete disablement of the logging support is very unlikely, as is the disablement of the `error` log level. +While `warn` and `info` log levels may be disabled in rare cases, they're usually not. +Accordingly, any reported cases on `error`, `warn`, and `debug` log levels should be considered for suppression. + +Especially, if the performance impact is neglectable, e.g. for variable references (no method call) or simple getter or setter methods, or if the log frequency is very limited, e.g. only during initialization or cleanup, the PMD warning can be suppressed using an inline comment as follows: +```java +logger.warn("log something {} and {}", method(), param); //NOPMD +``` + +If the logs are part of a logging-specific method, that is intentionally called (only) for logging specific aspects like machine specs or config values, the warning can be suppressed for the entire method like so: +```java +@SuppressWarnings("PMD.GuardLogStatement") +public logSpecs() { +logger.info("log something {} and {}", method(), param) +} +``` + +Suppressing warnings allows for easier identification and reversion once the PMD bug is resolved. + +#### Fluent Logging API + +If parameter substitution is insufficient, local variable creation is not suitable, and suppression is not desired, the fluent logging API can be utilized as follows: +```java +logger.atDebug().log("log something {} and {}", method(), param); +``` + +Please do not use the more verbose variant(s) utilizing the `setMessage()`, `addArgument()`, `addKeyValue()` and similar methods of the fluent logging API's [LoggingEventBuilder](https://www.slf4j.org/apidocs/org/slf4j/spi/LoggingEventBuilder.html) to keep complexity low and your log statements readable. + ## Testing Terasology's test suite can be found in the [engine-tests/src/test/java/org/terasology](https://github.com/MovingBlocks/Terasology/tree/develop/engine-tests/src/test/java/org/terasology) folder. From 1b480337c5bc193e0c1345c241d6348eaad44182 Mon Sep 17 00:00:00 2001 From: jdrueckert Date: Wed, 13 Mar 2024 15:24:22 +0100 Subject: [PATCH 3/3] doc: link TypeHandlerLibrary readme (#5222) --- docs/Serialization-Overview.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/Serialization-Overview.md b/docs/Serialization-Overview.md index 0c972d9f20d..0f73ecc83df 100644 --- a/docs/Serialization-Overview.md +++ b/docs/Serialization-Overview.md @@ -21,6 +21,8 @@ The `TypeHandlerLibrary` caches type handlers for various types. Since a single The `@RegisterTypeHandler` and the `@RegisterTypeHandlerFactory` annotations can also be used to register type handlers and type handler factories to the `TypeHandlerLibrary` in the `Context`. +[Read more about the TypeHandlerLibrary](https://github.com/MovingBlocks/Terasology/tree/develop/subsystems/TypeHandlerLibrary) + ### Examples #### `TypeHandler` without a `TypeHandlerFactory` @@ -50,4 +52,4 @@ Terasology currently supports JSON (via Gson) and binary (via Protobuf) serializ - Format independent -- code that serializes an object remains the same regardless of the target format. - Supports almost every possible type out-of-the-box. - Supports recursive types (i.e. types that refer to themselves) but not cyclic object references. -- It is possible to override default serialization behavior by specifying a new `TypeHandler` that handles a type differently. \ No newline at end of file +- It is possible to override default serialization behavior by specifying a new `TypeHandler` that handles a type differently.