Skip to content
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.

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

MergeMojo implementation based on MergeTask #126

Merged
merged 8 commits into from
Aug 27, 2013
Merged

MergeMojo implementation based on MergeTask #126

merged 8 commits into from
Aug 27, 2013

Conversation

hrmohr
Copy link
Contributor

@hrmohr hrmohr commented Aug 24, 2013

Same functionality as the MergeTask taking a list of FileSet as input with a single destFile output

@marchof
Copy link
Member

marchof commented Aug 25, 2013

@Godin Can you please quickly review this? This looks like a simple but valuable addition for our Maven support.

@marchof
Copy link
Member

marchof commented Aug 25, 2013

@hrmohr Thanks for the quick response! I'll try to get @Godin in the loop to get this reviewed asap. He is in charge of the JaCoCo Maven integration.

import java.util.List;

/**
* Mojo for merging a set of execution data store files into a single file
Copy link
Member

Choose a reason for hiding this comment

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

Documentation: The term is "execution data files (*.exec)" instead of "execution data store files"

@hrmohr
Copy link
Contributor Author

hrmohr commented Aug 25, 2013

I also updated the javadoc for ant task

@marchof
Copy link
Member

marchof commented Aug 25, 2013

@hrmohr Thx!

<fileSet>
<directory>${project.parent.build.directory}</directory>
<includes>
<include>jacoco2.exec</include>
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to use single fileSet here ( *.exec ) ?

@ghost ghost assigned Godin Aug 26, 2013
@Godin
Copy link
Member

Godin commented Aug 26, 2013

Guys, code looks good for me ( good job @hrmohr and thanks for contribution ), except of minor comments about integration test. @marchof I let you decide - merge as it is now and adjust integration test later or adjust before merge. Also we should not forget to update changelog.

@hrmohr
Copy link
Contributor Author

hrmohr commented Aug 26, 2013

Three times yes to your questions @Godin. Should I commit these changes to the integration test?

The input validation can also be improved because it wont fail if a valid fileset definition finds no execution data files for merging. It will only fail if no filesets are defined.
Perhaps it would also be nice if one could define a single fileset instead of a list with a single fileset.

@marchof
Copy link
Member

marchof commented Aug 27, 2013

@hrmohr Yes, please update the pull request directly.

I wouldn't fail if no files are found, maybe just log a info. So the plugin can be used in a generic way.

The list of filesets is probably more flexible.

@hrmohr
Copy link
Contributor Author

hrmohr commented Aug 27, 2013

I kept the list of filesets and handled missing execution files as suggested. Unless you want me to change anything this should be my last changes.

@marchof
Copy link
Member

marchof commented Aug 27, 2013

Thnak you! I'll going to merge this.

marchof added a commit that referenced this pull request Aug 27, 2013
MergeMojo implementation based on MergeTask
@marchof marchof merged commit 29cf17c into jacoco:master Aug 27, 2013
@Godin Godin mentioned this pull request Aug 27, 2013
@jacoco jacoco locked and limited conversation to collaborators Jan 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants