Clarify description for parameter "append" #1827
Conversation
State that by default execution data files are always written to in append mode. This closes jacoco#1676
| /** | ||
| * If set to true and the execution data file already exists, coverage data | ||
| * is appended to the existing file. If set to false, an existing execution | ||
| * data file will be replaced. | ||
| * data file will be replaced. The default behaviour is that execution data | ||
| * files are never cleared but coverage is always appended to an existing | ||
| * file. | ||
| */ | ||
| @Parameter(property = "jacoco.append") | ||
| Boolean append; |
There was a problem hiding this comment.
IMO rather than repeating
appended to the existing file
might be better to just simply explicitly specify default value:
index 06f798a6..a65a1ebe 100644
--- i/jacoco-maven-plugin/src/org/jacoco/maven/AbstractAgentMojo.java
+++ w/jacoco-maven-plugin/src/org/jacoco/maven/AbstractAgentMojo.java
@@ -55,11 +55,9 @@ public abstract class AbstractAgentMojo extends AbstractJacocoMojo {
/**
* If set to true and the execution data file already exists, coverage data
* is appended to the existing file. If set to false, an existing execution
- * data file will be replaced. The default behaviour is that execution data
- * files are never cleared but coverage is always appended to an existing
- * file.
+ * data file will be replaced.
*/
- @Parameter(property = "jacoco.append")
+ @Parameter(property = "jacoco.append", defaultValue = "true")
Boolean append;which will be rendered in documentation as following
with a link to
and in execution of
mvn org.jacoco:jacoco-maven-plugin:0.8.13-SNAPSHOT:help -Dgoal=prepare-agent -Ddetail
as
append (Default: true)
If set to true and the execution data file already exists, coverage data
is appended to the existing file. If set to false, an existing execution
data file will be replaced.
User property: jacoco.append
this also might be helpful for other tools that process this metadata - for example IntelliJ IDEA will be showing
There was a problem hiding this comment.
@Godin I would also prefer the formal specification of the defaultValue.
This holds true also for all other properties. The only issue is that we currently have default values in AgentOptions defined. So we should use those constants. Then all the != null checks in createAgentOptions()can be removed.
There was a problem hiding this comment.
I agree that default values should be defined in one place: Either on the Plugin/tool level or on the AgentOptions level. The former has the disadvantage that you need to define it for all tools (like Ant, CLI, ...) separately. Not sure how to even modify that for Gradle: https://docs.gradle.org/current/userguide/jacoco_plugin.html
There was a problem hiding this comment.
Would you merge if I simplify to @Parameter(property = "jacoco.append", defaultValue = "true") despite having the default value then specified multiple times?
There was a problem hiding this comment.
I just tried this. The duplication can be easily avoided by using an new constant AgentOptions.DEFAULT_APPEND.
But specifying default values actually has a side effect: The value is always appended to the agent configuration (even, if it the default).
There was a problem hiding this comment.
But specifying default values actually has a side effect: The value is always appended to the agent configuration (even, if it the default).
@marchof to me seems that this is avoidable as following:
index 15da074a..d8cda96a 100644
--- i/org.jacoco.core/src/org/jacoco/core/runtime/AgentOptions.java
+++ w/org.jacoco.core/src/org/jacoco/core/runtime/AgentOptions.java
@@ -51,6 +51,8 @@ public final class AgentOptions {
*/
public static final String APPEND = "append";
+ public static final boolean DEFAULT_APPEND = true;
+
/**
* Wildcard expression for class names that should be included for code
* coverage. Default is <code>*</code> (all classes included).
@@ -294,7 +296,11 @@ public final class AgentOptions {
* <code>true</code>, when the output should be appended
*/
public void setAppend(final boolean append) {
- setOption(APPEND, append);
+ if (append == DEFAULT_APPEND) {
+ options.remove(APPEND);
+ } else {
+ setOption(APPEND, append);
+ }
}or
index 7776e611..e3dad9d4 100644
--- i/jacoco-maven-plugin/src/org/jacoco/maven/AbstractAgentMojo.java
+++ w/jacoco-maven-plugin/src/org/jacoco/maven/AbstractAgentMojo.java
@@ -185,7 +185,7 @@ public abstract class AbstractAgentMojo extends AbstractJacocoMojo {
AgentOptions createAgentOptions() {
final AgentOptions agentOptions = new AgentOptions();
agentOptions.setDestfile(getDestFile().getAbsolutePath());
- if (append != null) {
+ if (append != AgentOptions.DEFAULT_APPEND) {
agentOptions.setAppend(append.booleanValue());
}
if (includes != null && !includes.isEmpty()) {There was a problem hiding this comment.
Exactly, I did such a PoC like this already. The only problem is that DEFAULT_APPENDmust be of type String, otherwise it cannot be assigned to the defaultValue annotation at compile time.
I just pushed my PoC here: #1915
State that by default execution data files are always written to in append mode.
This closes #1676