-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add new attribute "sourcefilename" to "class" element in XML report #702
Conversation
<li>The XML report now has an optional attribute <code>sourcefilename</code> | ||
on the <code>class</code> element to allow unambiguously relate classes | ||
to source files. The JaCoCo DTD version has been updated to 1.1. | ||
(GitHub <a href="https://github.com/jacoco/jacoco/issues/701">#701</a>).</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed in #668 (comment) - existing links point to PRs, not on original issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx, fixed.
<ul> | ||
<li>The XML report now has an optional attribute <code>sourcefilename</code> | ||
on the <code>class</code> element to allow unambiguously relate classes | ||
to source files. The JaCoCo DTD version has been updated to 1.1. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently existing items consisting of multiple sentences use only one dot after link to GitHub in the last sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx, fixed.
@@ -44,6 +44,8 @@ | |||
<!ELEMENT class (method*, counter*)> | |||
<!-- fully qualified VM name --> | |||
<!ATTLIST class name CDATA #REQUIRED> | |||
<!-- name of the corresponding source file --> | |||
<!ATTLIST class sourcefilename CDATA #IMPLIED> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My eyes are bleeding a bit as a reaction on readability of this attribute name. But this is consistent with already existing other attributes that don't use CamelCase, what a pity that XML is case sensitive.
Wondering if better to keep it consistent with Java API or simplify into e.g. source
, sourcefile
? As a note - attribute of class denoting name of source file has name SourceFile
in JVMS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same thoughts but then decided for a consistent naming:
- In our API the attribute is also called sourceFileName (ok in camel case)
- Also it is conistent with the XML schema itself: It makes a reference to sourcefile/@name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
@@ -38,6 +38,14 @@ <h3>Fixed Bugs</h3> | |||
(GitHub <a href="https://github.com/jacoco/jacoco/issues/672">#672</a>).</li> | |||
</ul> | |||
|
|||
<h3>API Changes</h3> | |||
<ul> | |||
<li>The XML report now has an optional attribute <code>sourcefilename</code> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about addition of note that for better forward compatibility we recommend consumers of XML report to not fail (ignore / warn) on unknown attributes / nodes ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether our change log is the right place to advocate robust interface design line tolerant reader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, you right - not really a correct place.
@@ -32,7 +32,7 @@ | |||
*/ | |||
public class XMLFormatter { | |||
|
|||
private static final String PUBID = "-//JACOCO//DTD Report 1.0//EN"; | |||
private static final String PUBID = "-//JACOCO//DTD Report 1.1//EN"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI @marchof I triggered works on changing SonarQube integration from reading of binary to XML - https://jira.sonarsource.com/browse/MMF-1362 And for the time being seems that no changes will be required on JaCoCo side. Was thinking that maybe with increasing adoption of XML report, we'll need to introduce something like checksum of source file so that consumer can check it, however as of now I'm not aware of problems with mismatches in case of imports from other coverage tools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Can we handle this in a separate PR once someone comes up with a real use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not request of change / addition - just wanted to share thought with you. Let's handle this later when / if we'll see emerging need for real 😉
This allows to unambiguously relate classes to source files in case of multiple top level classes.
Due to review comment by @Godin.
@Godin Rebased on master. So should me mergeable now. |
Was about to resolve conflict by myself, but you've been faster 😆 Thanks! |
A new attribute should be added to allow mapping source files with secondary classes.
Fixes #701