8000 Clarify description for parameter "append" by kwin · Pull Request #1827 · jacoco/jacoco · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Clarify description for parameter "append" #1827

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Clarify description for parameter "append" #1827

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion jacoco-maven-plugin/src/org/jacoco/maven/AbstractAgentMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +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.
* 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;
Comment on lines 55 to 63
Copy link
Member
@Godin Godin Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screenshot 2025-01-10 at 15 19 52

with a link to

Screenshot 2025-01-10 at 15 20 27

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

Screenshot 2025-01-10 at 15 33 39

@kwin @marchof WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author
@kwin kwin Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down
0