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

Allow doctype declaration for JaCoCo #18

Merged
merged 2 commits into from
Jul 6, 2014

Conversation

gesellix
Copy link
Contributor

@gesellix gesellix commented Jul 6, 2014

After the fix for Cobertura as in #16, I now have the same issue for JaCoCo. See https://travis-ci.org/gesellix-docker/docker-client/builds/29244127 for an example.

This PR extracts the XmlParser and its configuration into a factory to avoid future inconsistencies.

Some unused imports have been removed, too :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling a67c75d on gesellix:allow-DOCTYPE-declaration-jacoco into 874038a on kt3k:master.

@kt3k
Copy link
Owner

kt3k commented Jul 6, 2014

Hi! @gesellix

Thanks for the contribution again!

It seems a nice refactoring. So I'll merge it.

But because I'd like to follow the guideline of Domain Driven Design (of Eric Evans) as far as possible, maybe I'm going to change some file paths a little later.

Thanks,

kt3k added a commit that referenced this pull request Jul 6, 2014
@kt3k kt3k merged commit 820e919 into kt3k:master Jul 6, 2014
@gesellix
Copy link
Contributor Author

gesellix commented Jul 6, 2014

I guess following the DDD principle won't be a bad idea, so I don't have a problem with changing paths. Thanks for merging! Just as a side note: I still don't understand where the issue really comes from, on my system the default is as expected, only on travis-ci the XmlParser seems to have another config.

@kt3k
Copy link
Owner

kt3k commented Jul 6, 2014

Thanks for the note. Hmm... maybe something is wrong with xmlparser default settings in travis-ci?

By the way I deployed a version including this fix as v0.6.1.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants