#135 Added schema location to metric.xml #196

llorllale · 2018-02-18T14:11:36Z

as per #135:

Added xsi:noNamespaceSchemaLocation to the generated metric.xml and left puzzles for the rest of the schemas in src/resources/org/jpeek/xsd

0crat · 2018-02-18T14:11:57Z

Job #196 is now in scope, role is REV

0crat · 2018-02-18T14:14:53Z

This pull request #196 is assigned to @amihaiemil /z, here is why. The budget is 15 minutes, see §4. Please, read §27 and when you decide to accept the changes, inform @yegor256 /z (the architect) right in this ticket. If you decide that this PR should not be accepted ever, also inform the architect.

codecov-io · 2018-02-18T14:17:37Z

Codecov Report

Merging #196 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #196      +/-   ##
============================================
+ Coverage     74.41%   74.46%   +0.05%     
  Complexity      163      163              
============================================
  Files            31       31              
  Lines          1020     1022       +2     
  Branches         73       73              
============================================
+ Hits            759      761       +2     
  Misses          231      231              
  Partials         30       30

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/org/jpeek/Report.java	`93.33% <100%> (+0.31%)`	`6 <0> (ø)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2c555f...752b4d5. Read the comment docs.

llorllale · 2018-02-18T14:19:03Z

@amihaiemil please review

llorllale · 2018-02-19T15:22:40Z

@amihaiemil just fixed some conflicts, please review

amihaiemil

@llorllale I think you merged some branches the wrong way? This PR has a lot of stuff not concerning the parent ticket.

amihaiemil · 2018-02-20T12:58:08Z

src/main/java/org/jpeek/App.java

@@ -131,7 +131,8 @@ public App(final Path source, final Path target) {
                new MapEntry<>("LCOM5", true),
                new MapEntry<>("SCOM", true),
                new MapEntry<>("NHD", true),
-                new MapEntry<>("MMAC", true)
+                new MapEntry<>("MMAC", true),
+                new MapEntry<>("OCC", true)


@llorllale Why did you add OCC here? Does it have to do with metric.xsl?

@amihaiemil fixed; this was an error on my part

amihaiemil · 2018-02-20T12:59:35Z

src/main/java/org/jpeek/Report.java

    Report(final XML xml, final String name,
        final Map<String, Object> args,
        final double mean, final double sigma) {
        this.skeleton = xml;
        this.metric = name;
-        this.params = args;
+        this.params = new MapOf<>(
+            // @checkstyle LineLength (1 line)


@llorllale Can you indent this code so you don't have to escape checkstyle?

@amihaiemil ~~fixed~~ wait

amihaiemil · 2018-02-20T12:59:44Z

src/main/java/org/jpeek/Report.java

+                    new UncheckedText(
+                        new TextOf(
+                            new InputOf(
+                                // @checkstyle LineLength (1 line)


@llorllale Same here

@amihaiemil ~~fixed~~ wait

amihaiemil · 2018-02-20T13:00:52Z

src/main/resources/org/jpeek/metrics/OCC.xsl

@@ -0,0 +1,107 @@
+<?xml version="1.0"?>


@llorllale Again, I don't understand why this PR adds both OCC metric and implements the metric.xsl thing?

@amihaiemil I fixed this

amihaiemil · 2018-02-20T13:02:41Z

src/main/resources/org/jpeek/xsd/index.xsd

@@ -22,6 +22,14 @@ LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 SOFTWARE.
 -->
+<!--
+@todo #134:30min Index.xsd: add `xsd:documentation` tags as per issue #134. We need to


@llorllale Why are you adding these todos here? They have nothing to do with the ticket or the metric.xsl functionality, right?

@amihaiemil fixed, they had nothing to do with the ticket

llorllale · 2018-02-20T13:20:04Z

@amihaiemil fixed, please review.

You were right. Lucky for me however, those other changes are now in master so it's all good. :)

As per PR review: * fixed indentation in Report.java

llorllale · 2018-02-20T18:08:10Z

@amihaiemil fixed the indentation issues

amihaiemil · 2018-02-20T19:01:32Z

@rultor good to merge

rultor · 2018-02-20T19:02:18Z

@rultor good to merge

@amihaiemil Thanks for your request. @yegor256 Please confirm this.

amihaiemil · 2018-02-21T07:48:17Z

@yegor256 ping

yegor256 · 2018-02-21T12:41:05Z

src/main/java/org/jpeek/Report.java

+                        new TextOf(
+                            new InputOf(
+                                Report.class.getResourceAsStream(
+                                    "xsl/metric-post-schemaloc.xsl"


@llorllale looks weird. Can't we just add this single attribute to the skeleton.xml when we are building it? Why making this simple operation so complex?

@yegor256 My line of reasoning is: Report creates metric.xml so it is responsible for properly building it.

Skeleton creates the skeleton.xml so I believe it is responsible for properly building that xml.

I see that App is currently creating both index.xml and matrix.xml and I think those two things should be extracted into their own classes who in turn will be responsible for properly building those xmls.

WDYT?

@yegor256 let's also remember that the value for xsi:noNamespaceSchemaLocation will be different among the files.

@llorllale why can't you set it via Xembly? Why using XSL?

@yegor256

why can't you set it via Xembly?

Xembler works with nodes. Can't set the required attribute on the node returned by the XML resulting from transforming the skeleton into metric because it's a document node (throws error "Cannot cast ...DocumentImpl to ...Element"). I think I would have to bend over backwards in the code to make it work (appending child elements, etc)

Why using XSL?

We already have a post-processing stage in Report so I took advantage of that. Also, this way seems cleaner to me than the alternative I described above. The noise can be reduced a little if XSLDocument had a ctor that accepted InputStream and params together.

yegor256 · 2018-02-21T12:41:39Z

src/main/resources/org/jpeek/xsd/index.xsd

@@ -26,6 +26,9 @@ SOFTWARE.
 @todo #134:30min Index.xsd: add `xsd:documentation` tags as per issue #134. We need to
 document the schemas in src/resources/org/jpeek/xsd so that the semantics of
 the generated XML documents is transparent for maintainers.
+@todo #135:30min Index.xsd: add a reference to this schema from the generated
+ skeleton.xml. Schemas from src/resources/org/jpeek/xsd must be referenced in


@llorllale what skeleton.xml has to do with this file? Typo?

@yegor256 yes, typo

yegor256 · 2018-02-21T12:41:44Z

src/main/resources/org/jpeek/xsd/matrix.xsd

@@ -26,6 +26,9 @@ SOFTWARE.
 @todo #134:30min Matrix.xsd: add `xsd:documentation` tags as per issue #134. We need to
 document the schemas in src/resources/org/jpeek/xsd so that the semantics of
 the generated XML documents is transparent for maintainers.
+@todo #135:30min Matrix.xsd: add a reference to this schema from the generated
+ skeleton.xml. Schemas from src/resources/org/jpeek/xsd must be referenced in


@llorllale what skeleton.xml has to do with this file? Typo?

@yegor256 yes, typo

yegor256 · 2018-02-21T12:41:49Z

src/main/resources/org/jpeek/xsd/skeleton.xsd

@@ -22,6 +22,11 @@ LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
 OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 SOFTWARE.
 -->
+<!--
+@todo #135:30min Skeleton.xsd: add a reference to this schema from the generated
+ skeleton.xml. Schemas from src/resources/org/jpeek/xsd must be referenced in


@llorllale what skeleton.xml has to do with this file? Typo?

@yegor256 ~~yes, typo~~ actually, NO, this one was not a typo :)

yegor256 · 2018-02-21T12:42:07Z

@llorllale it seems you over-designed it, see above

Fixed typos

amihaiemil · 2018-02-22T08:39:10Z

@llorllale Are you done with the changes asked by Yegor? If so, ping him so we can try to merge

llorllale · 2018-02-22T09:46:40Z

Ping @yegor256

llorllale · 2018-02-22T16:45:09Z

@yegor256 any thoughts ?

yegor256 · 2018-02-22T17:37:48Z

@llorllale see above

llorllale · 2018-02-23T19:21:09Z

@yegor256 please see my reply above. Also, please let me know your thoughts regarding the puzzle I'm leaving here as it also involves a design decision.

amihaiemil · 2018-02-26T12:47:47Z

@yegor256 ping

0crat · 2018-02-26T14:15:27Z

@amihaiemil /z this job was assigned to you 8 days ago. It will be taken away from you soon, unless you close it, see §8. Read this and this, please.

amihaiemil · 2018-02-26T15:19:17Z

@yegor256 ping

yegor256 · 2018-02-26T18:31:59Z

@llorllale it still seems weird to me that we are using XSL just in order to add one attribute to the XML. We have to use Xembly, it will take just one instruction and the problem is solved. Current solution is too complex and verbose.

llorllale · 2018-02-26T20:57:52Z

@yegor256 where would this one instruction go? Sorry, for asking, I don't see it.

amihaiemil · 2018-02-27T08:19:43Z

@llorllale @yegor256 If you can't agree on the solution, then maybe you should open another PR and solve it there, since this is in my hands for 8 days now :D

llorllale · 2018-02-27T13:16:14Z

@amihaiemil agreed, should I just close this?

amihaiemil · 2018-02-27T13:19:25Z

@llorllale yes. I mean, I can't think of a better solution for this kind of situation. I, as the CR, did my job, but clock is ticking. I was thinking that now the project will have to pay for a second CR -- but then again, if I would continue to review this one, I would spend more than 15min so it wouldn't be correct.

So yeah, I think this is the best solution, just close this and open another one, discuss further changes there :)

llorllale · 2018-02-27T13:46:52Z

@yegor256 the solution I've provided works and moves the project forward. If an improvement needs to be made then I understand we can leave a puzzle for it. Agree?

This job has gone beyond budget for both DEV and REV.

yegor256 · 2018-02-27T16:33:25Z

@llorllale sure, you can always "puzzle" the problem to cut corners and close the ticket.

As per ARC review: * Left puzzle for further refactor towards simpler solution

llorllale · 2018-02-27T16:52:41Z

@amihaiemil @yegor256 I've changed the puzzle; now leaving the refactoring work as the puzzle

amihaiemil · 2018-02-28T12:18:12Z

@yegor256 can we merge this now, it's the 10th day, 0crat will take it away tonight, I think :(

0crat · 2018-02-28T14:17:09Z

The user @amihaiemil /z resigned from #196, please stop working. Reason for job resignation: It is older than 10 days, see §8

0crat · 2018-02-28T14:17:14Z

Resigned on delay, see §8: -15 points just awarded to @amihaiemil /z, total is +1145

0crat · 2018-02-28T14:17:49Z

Mistake of @amihaiemil /z /z (your student): Resigned on delay, see §8: -15 points just awarded to @yegor256 /z, total is +11029

yegor256 · 2018-02-28T17:06:15Z

@rultor merge

rultor · 2018-02-28T17:07:37Z

@rultor merge

@yegor256 OK, I'll try to merge now. You can check the progress of the merge here

0crat · 2018-02-28T17:18:47Z

The job #196 is now out of scope

rultor · 2018-02-28T17:19:04Z

@rultor merge

@yegor256 Done! FYI, the full log is here (took me 11min)

llorllale added 2 commits February 18, 2018 10:09

(cqfn#135) Added schema location to metric.xml

23e5c58

Merge branch 'master' of https://github.com/yegor256/jpeek into 135

35e9b2b

0crat added the scope label Feb 18, 2018

Merge branch '135' of https://github.com/llorllale/jpeek into 135

6a32977

amihaiemil reviewed Feb 20, 2018

View reviewed changes

Merge branch 'master' of https://github.com/yegor256/jpeek into 135

a4ddaf4

(cqfn#135) Added schema location to metric.xml

e745f72

As per PR review: * fixed indentation in Report.java

yegor256 reviewed Feb 21, 2018

View reviewed changes

(cqfn#135) Added schema location to metric.xml

8811e77

Fixed typos

llorllale added 2 commits February 27, 2018 12:40

Merge branch 'master' of https://github.com/yegor256/jpeek into 135

63c7147

(cqfn#135) Added schema location to metric.xml

752b4d5

As per ARC review: * Left puzzle for further refactor towards simpler solution

rultor merged commit 752b4d5 into cqfn:master Feb 28, 2018

0crat removed the scope label Feb 28, 2018

llorllale mentioned this pull request Mar 1, 2018

Schemas from src/resources/org/jpeek/xsd must be referenced in generated XMLs #135

Closed

gsnoff mentioned this pull request Apr 23, 2019

Optimal way of making Xembler work with Report is not obvious #322

Closed

llorllale deleted the 135 branch April 23, 2019 11:21

#135 Added schema location to metric.xml #196

#135 Added schema location to metric.xml #196

Conversation

llorllale commented Feb 18, 2018 • edited Loading

0crat commented Feb 18, 2018

0crat commented Feb 18, 2018

codecov-io commented Feb 18, 2018 • edited Loading

Codecov Report

llorllale commented Feb 18, 2018

llorllale commented Feb 19, 2018

amihaiemil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorllale Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorllale Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorllale commented Feb 20, 2018

llorllale commented Feb 20, 2018

amihaiemil commented Feb 20, 2018

rultor commented Feb 20, 2018

amihaiemil commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorllale Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llorllale Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

yegor256 commented Feb 21, 2018

amihaiemil commented Feb 22, 2018

llorllale commented Feb 22, 2018

llorllale commented Feb 22, 2018

yegor256 commented Feb 22, 2018

llorllale commented Feb 23, 2018

amihaiemil commented Feb 26, 2018

0crat commented Feb 26, 2018

amihaiemil commented Feb 26, 2018

yegor256 commented Feb 26, 2018

llorllale commented Feb 26, 2018

amihaiemil commented Feb 27, 2018

llorllale commented Feb 27, 2018

amihaiemil commented Feb 27, 2018

llorllale commented Feb 27, 2018

yegor256 commented Feb 27, 2018

llorllale commented Feb 27, 2018

amihaiemil commented Feb 28, 2018

0crat commented Feb 28, 2018

0crat commented Feb 28, 2018

0crat commented Feb 28, 2018

yegor256 commented Feb 28, 2018

rultor commented Feb 28, 2018

0crat commented Feb 28, 2018

rultor commented Feb 28, 2018

llorllale commented Feb 18, 2018 •

edited

Loading

codecov-io commented Feb 18, 2018 •

edited

Loading

llorllale Feb 20, 2018 •

edited

Loading

llorllale Feb 20, 2018 •

edited

Loading

llorllale Feb 23, 2018 •

edited

Loading

llorllale Feb 21, 2018 •

edited

Loading