Skip to content

New API org.jacoco.core.tools #159

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

Merged
merged 2 commits into from
Nov 16, 2013
Merged

New API org.jacoco.core.tools #159

merged 2 commits into from
Nov 16, 2013

Conversation

marchof
Copy link
Member

@marchof marchof commented Nov 16, 2013

Create new API package org.jacoco.core.tools for high level tools shared by different JaCoCo integrations.

@buildhive
Copy link

Java Code Coverage Tools » jacoco #155 SUCCESS
This pull request looks good
(what's this?)

* <code>reset==false</code>, <code>retryCount==0</code> and
* <code>retryDelay=1000</code>.
*/
public ExecDumpClient() {
Copy link
Member

Choose a reason for hiding this comment

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

I would like to see another constructor with all arguments given to make them final instead of providing setters. The only advantage of using setters here is that you may not mix arguments unintentionally.

Copy link
Member Author

Choose a reason for hiding this comment

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

While I'm also a fan of immutable objects I prefer this solution here for the following reasons:

  • Avoid too many parameters for an constructor
  • provide out-of-the box default behavior
  • extensible API for additional attributes in future.

@buildhive
Copy link

Java Code Coverage Tools » jacoco #156 SUCCESS
This pull request looks good
(what's this?)

@mfriedenhagen
Copy link
Member

I really like the ExecDumpClient as all socket related stuff is hidden. Looks fine to me.

@marchof
Copy link
Member Author

marchof commented Nov 16, 2013

@mfriedenhagen Thanks for reviewing this!

marchof added a commit that referenced this pull request Nov 16, 2013
New API org.jacoco.core.tools
@marchof marchof merged commit 6a6317b into master Nov 16, 2013
@marchof marchof deleted the issue-159 branch November 16, 2013 21:52
@marchof marchof restored the issue-159 branch November 23, 2013 07:22
@marchof marchof deleted the issue-159 branch November 23, 2013 07:26
@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.

3 participants