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

Conversation

Projects
None yet
3 participants
@hrmohr
Contributor

hrmohr commented Aug 24, 2013

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

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Aug 25, 2013

Please just add

Mads Mohr Christensen - initial API and implementation

and remove the others as this file completely is your contribution.

Please just add

Mads Mohr Christensen - initial API and implementation

and remove the others as this file completely is your contribution.

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Aug 25, 2013

To avoid code duplication I just added the method ExecFileLoader.save() on master (for usage see MergeTask). To use this you can merge jacoco master into you branch. This pull request should get updated automatically as soon as you push your merged and adapter branch again. Thx!

To avoid code duplication I just added the method ExecFileLoader.save() on master (for usage see MergeTask). To use this you can merge jacoco master into you branch. This pull request should get updated automatically as soon as you push your merged and adapter branch again. Thx!

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Aug 25, 2013

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Aug 25, 2013

Member

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

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.

@hrmohr

This comment has been minimized.

Show comment
Hide comment
@hrmohr

hrmohr Aug 25, 2013

Contributor

I also updated the javadoc for ant task

Contributor

hrmohr commented Aug 25, 2013

I also updated the javadoc for ant task

@marchof

This comment has been minimized.

Show comment
Hide comment
@marchof
Member

marchof commented Aug 25, 2013

@hrmohr Thx!

@ghost ghost assigned Godin Aug 26, 2013

@Godin

This comment has been minimized.

Show comment
Hide comment
@Godin

Godin Aug 26, 2013

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@hrmohr

hrmohr Aug 26, 2013

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Aug 27, 2013

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@hrmohr

hrmohr Aug 27, 2013

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@marchof

marchof Aug 27, 2013

Member

Thnak you! I'll going to merge this.

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

Merge pull request #126 from hrmohr/MergeMojo
MergeMojo implementation based on MergeTask

@marchof marchof merged commit 29cf17c into jacoco:master Aug 27, 2013

1 check passed

default The Travis CI build passed
Details

@Godin Godin referenced this pull request Aug 27, 2013

Closed

Merge multiple data files #12

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