(#227) Refactor setting schema reference attribute #326
Conversation
|
Job #326 is now in scope, role is |
Codecov Report
@@ Coverage Diff @@
## master #326 +/- ##
===========================================
+ Coverage 75.58% 75.79% +0.2%
Complexity 184 184
===========================================
Files 34 34
Lines 1102 1099 -3
Branches 88 87 -1
===========================================
Hits 833 833
+ Misses 238 236 -2
+ Partials 31 30 -1
Continue to review full report at Codecov.
|
|
This pull request #326 is assigned to @ilyakharlamov/z, here is why; the budget is 15 minutes, see §4; please, read §27 and when you decide to accept the changes, inform @paulodamaso/z (the architect) right in this ticket; if you decide that this PR should not be accepted ever, also inform the architect; this blog post will help you understand what is expected from a code reviewer; there will be no monetary reward for this job |
| this.params | ||
| ).transform(this.skeleton); | ||
| return new XMLDocument( | ||
| new Xembler( |
There was a problem hiding this comment.
In this class, sometimes resources are accessed viathis.getClass().getResource() sometimes via Report.class.getResource(). Need to decide which way you want to use and stick to it, so it would be uniform across the whole class (either one way or another).
There was a problem hiding this comment.
@ilyakharlamov Fixed. Turns out the this.getClass() form isn’t used anywhere else in the project.
| Sources.DUMMY, | ||
| this.params | ||
| ).transform(this.skeleton).node() | ||
| ) |
There was a problem hiding this comment.
If we replace that with just applyQuietly( new XSLDocument( this.getClass().getResourceAsStream(name) ).transform(this.skeleton).node() ) all the unit test pass just fine. This means that either this.params is not needed or isn't tested. So we need to either remove this.params at all, or add a unit test that checks whether those params really work.
There was a problem hiding this comment.
@ilyakharlamov Currently only C3.xsl and OCC.xsl metric templates in src/main/resources/org/jpeek/metrics have a <xsl:param/> tag defining a parameter labeled ctors, and it seems that OCC.xsl doesn’t even use the parameter. C3.xsl is a work in progress, see #175.
Maybe leave a puzzle here or in ReportTest.java describing the impediment?
There was a problem hiding this comment.
@ilyakharlamov FYI, added the puzzle.
src/main/java/org/jpeek/Report.java
Outdated
| * 'metric.xsd' in the generated 'metric.xml' is too complex for | ||
| * the task at hand. Refactor towards a simpler solution that | ||
| * ideally would just require one or two Xembly instructions to | ||
| * add the required attribute. | ||
| */ | ||
| @SuppressWarnings("unchecked") |
There was a problem hiding this comment.
I don't see a need for @SuppressWarnings("unchecked") here.
There was a problem hiding this comment.
@ilyakharlamov Fixed.
|
@ilyakharlamov Please see my comments in the review above. |
src/main/java/org/jpeek/Report.java
Outdated
| ) | ||
| ).applyQuietly( | ||
| new XSLDocument( | ||
| new TextOf(res).asString(), |
There was a problem hiding this comment.
Replace that with OO: new TextOf( new ResourceOf( new FormattedText( "org/jpeek/metrics/%s.xsl", this.metric ) ) ).asString(), , everything above, all those ifs, can be removed.
There was a problem hiding this comment.
@ilyakharlamov Done.
Also apply stylistic fixes suggested in reviews to the pull request.
Done. |
|
@paulodamaso good to merge |
|
@ilyakharlamov There also might be a possible optimization of putting the |
|
@Protagores there are many, dozens or even hundreds of performance optimizations are possible here but sometimes it can make the code less readable, harder to test, etc. So quite often, when choosing between the performance and reliability, you can only pick one or another but not both. Those projects focus on OO and clarity, not performance as you may notice. That's a topic for another conversation. |
|
@Protagores and @ilyakharlamov , thanks for the solution and the discussion above! |
|
@rultor merge |
@paulodamaso OK, I'll try to merge now. You can check the progress of the merge here |
@paulodamaso Done! FYI, the full log is here (took me 15min) |
|
@fabriciofx/z please review this job completed by @ilyakharlamov/z, as in §30; the job will be fully closed and all payments will be made when the quality review is completed |
|
The job #326 is now out of scope |
|
Job #326 is not in the agenda of @ilyakharlamov/z, can't inspect |
|
There is an unrecoverable failure on my side. Please, submit it here: 0.46.9: CID: 58e30012-eadf-4966-8c10-c17b3c524e44, Type: "Make payment" |
|
Payment to |
|
@0crat quality acceptable |
|
Order was finished, quality is "acceptable": +15 point(s) just awarded to @ilyakharlamov/z |
|
Quality review completed: +4 point(s) just awarded to @fabriciofx/z |
As per #227:
Refactored the addition of
xsi:noNamespaceSchemaLocationto generatedmetric.xmlinto applying a few Xembly instructions instead of an XSL transformation.Three instructions were eventually needed—one to point the cursor to the
metricelement (since it points above the root element by default), one to declare the XML namespace of the schema location attribute, and one to set the location itself.