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

(#106) Skeleton should track method-method interactions #153

Merged
merged 7 commits into from Feb 12, 2018

Conversation

Projects
None yet
6 participants
@llorllale
Contributor

llorllale commented Feb 3, 2018

as per #106

@0crat

This comment has been minimized.

Collaborator

0crat commented Feb 3, 2018

@yegor256/z please, pay attention to this pull request

@0crat 0crat added the scope label Feb 3, 2018

@0crat

This comment has been minimized.

Collaborator

0crat commented Feb 3, 2018

Job #153 is now in scope, role is REV

@codecov-io

This comment has been minimized.

codecov-io commented Feb 3, 2018

Codecov Report

Merging #153 into master will increase coverage by 0.24%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #153      +/-   ##
============================================
+ Coverage     74.34%   74.59%   +0.24%     
  Complexity      127      127              
============================================
  Files            27       27              
  Lines           920      929       +9     
  Branches         57       57              
============================================
+ Hits            684      693       +9     
  Misses          211      211              
  Partials         25       25
Impacted Files Coverage Δ Complexity Δ
src/main/java/org/jpeek/Skeleton.java 93.84% <100%> (+0.45%) 16 <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 a9913a1...6a7aa75. Read the comment docs.

@0crat

This comment has been minimized.

Collaborator

0crat commented Feb 3, 2018

This pull request #153 is assigned to @amihaiemil/z. 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.

@llorllale

This comment has been minimized.

Contributor

llorllale commented Feb 4, 2018

@amihaiemil please review

@@ -344,6 +351,23 @@ public void visitFieldInsn(final int opcode,
}
Skeleton.Visitor.this.dirs.set(attr).up().up().up().up();
}
@Override
public void visitMethodInsn(final int opcode,

This comment has been minimized.

@amihaiemil

amihaiemil Feb 4, 2018

Contributor

@llorllale what does insn mean? Can you leave a JavaDoc over this method?

This comment has been minimized.

@llorllale

llorllale Feb 4, 2018

Contributor

insn stands for "instruction" - we're overriding MethodVisitor.visitMethodInsn.

Given @Override and this explanation, do you still feel a javadoc is needed?

@@ -332,7 +339,7 @@ public void visitFieldInsn(final int opcode,
"methods/method[@name='%s' and @desc='%s']",
mtd, desc
)
).strict(1).addIf("ops").add("op");

This comment has been minimized.

@amihaiemil

amihaiemil Feb 4, 2018

Contributor

@llorllale why did you remove strict(1)? :D

@@ -64,6 +64,15 @@ SOFTWARE.
</xs:sequence>
</xs:complexType>
</xs:element>
<!--
@todo #106:30min Refactor <ops>.

This comment has been minimized.

@amihaiemil

amihaiemil Feb 4, 2018

Contributor

@llorllale Why does the <method_calls> element have to be inside <ops> -- what is ops, actually? To me it seems better if it is inside element <method>, as you did it now.

This comment has been minimized.

@llorllale

llorllale Feb 4, 2018

Contributor

method/ops [should] contains all operations of the method, which includes calls to methods.

This comment has been minimized.

@yegor256

yegor256 Feb 6, 2018

Owner

@llorllale I agree with @amihaiemil , this design looks weird. Let's go with this:

<ops>
  <op code="get">attr1</op>
  <op code="set">attr2</op>
  <op code="call">the_method_name</op> <!-- this is the new "code" we are introducing -->
</ops>

This comment has been minimized.

@llorllale

llorllale Feb 6, 2018

Contributor

@yegor256 your proposal is missing the fqn of the class that the method belongs to. Where does it go?

(#106) Skeleton should track method-method interactions
As per PR review:
- Added strict check on XML for method visitor
@llorllale

This comment has been minimized.

Contributor

llorllale commented Feb 4, 2018

@amihaiemil I've added the strict checks - thanks for pointing that out

@amihaiemil

This comment has been minimized.

Contributor

amihaiemil commented Feb 4, 2018

@rultor merge

@rultor

This comment has been minimized.

Collaborator

rultor commented Feb 4, 2018

@rultor merge

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

@amihaiemil

This comment has been minimized.

Contributor

amihaiemil commented Feb 6, 2018

@yegor256 ping

@yegor256

This comment has been minimized.

Owner

yegor256 commented Feb 6, 2018

@llorllale see above

@amihaiemil

This comment has been minimized.

Contributor

amihaiemil commented Feb 9, 2018

@llorllale How are we here, what's the status?

@0crat

This comment has been minimized.

Collaborator

0crat commented Feb 9, 2018

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

@amihaiemil

This comment has been minimized.

Contributor

amihaiemil commented Feb 9, 2018

@yegor256 here, for instance; we're still waiting for the DEV to do the last changes. What can I do to speed things up and not lose the task?

@llorllale

This comment has been minimized.

Contributor

llorllale commented Feb 9, 2018

@amihaiemil first of all, the policy says that the ten days rule does not apply to code reviews, so it looks like a bug in 0crat.

Second, this situation threw me in a loop because REV had already approved the PR but ARC overrode with his objection, so it took me a little more time to decide what to do. Also, I asked the ARC a question which is still unanswered.

The ticket reporter says he's fine with something like this which I don't like because it lacks structure:

<op code="call">java.lang.String.substring</op>

What I'm going to do in a couple of hours is try this instead:

<op code="call" owner="fully.qualified.name.of.owner.Class">method_name</op>

I'll try to see if not too many tests break, and will leave a puzzle or something saying that #156 needs to be fixed for field access.

Btw I'm not too happy with the semantics of my proposal either. I strongly feel that the whole <ops> structure needs reworking, which is why I originally left a puzzle in skeleton.xsd for that

WDYT?

@amihaiemil

This comment has been minimized.

Contributor

amihaiemil commented Feb 9, 2018

@llorllale

first of all, the policy says that the ten days rule does not apply to code reviews, so it looks like a bug in 0crat.

Nope, it's not a bug now. It used to be a bug, but not anymore, it was introduced just now here.

I don't know exactly, I did not follow the discussion. But if the ARCH or someone else does not answer to your question, just be shameless and ping them constantly, spam them until they answer. They will, eventually :D

@yegor256

This comment has been minimized.

Owner

yegor256 commented Feb 9, 2018

@llorllale if you don't know what to do or don't have time - do something and leave it as is, with a puzzle. Just don't wait. Don't aim for perfection, aim for speed. That's the main rule.

Aim for speed, not perfection! (I will write a blog post about it soon)

(#106) Skeleton should track method-method interactions
As per further PR review:
- Now including info in a regular <op> tag
@llorllale

This comment has been minimized.

Contributor

llorllale commented Feb 9, 2018

@amihaiemil please review. A few notes:

  • left a puzzle in MetricsTest because adding that extra <op> breaks some tests
  • made small, trivial fix in SkeletonTest.createsXml to make it work with the addition of the new <op>
@llorllale

This comment has been minimized.

Contributor

llorllale commented Feb 9, 2018

@yegor256 thanks for the advice. It's not the first time I've been told that. Reading the points enumerated here helps a lot.

Having said that, I still have my reservations with this solution, but we'll leave it for another day, another ticket.

@llorllale

This comment has been minimized.

Contributor

llorllale commented Feb 10, 2018

@amihaiemil

This comment has been minimized.

Contributor

amihaiemil commented Feb 10, 2018

@llorllale looks ok

@amihaiemil

This comment has been minimized.

Contributor

amihaiemil commented Feb 10, 2018

@rultor merge

@rultor

This comment has been minimized.

Collaborator

rultor commented Feb 10, 2018

@rultor merge

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

(#106) Skeleton should track method-method interactions
Forgot to remove unnecessary element declaration from skeleton.xsd
@llorllale

This comment has been minimized.

Contributor

llorllale commented Feb 10, 2018

@amihaiemil sorry, I had forgotten to remove the old <method_calls> element definition from skeleton.xsd. Please check

@amihaiemil

This comment has been minimized.

Contributor

amihaiemil commented Feb 10, 2018

@llorllale looks ok. And you don't need to add something new to the XSD regarding the extra <op>, right? (I'm not so familiar with XSD/XSLT, sorry if it's a noob question)

@rultor

This comment has been minimized.

Collaborator

rultor commented Feb 10, 2018

@rultor merge

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

@amihaiemil

This comment has been minimized.

Contributor

amihaiemil commented Feb 10, 2018

@yegor256 Can we merge this now? I think until Monday 0crat will try to take it away from me :)

@llorllale

This comment has been minimized.

Contributor

llorllale commented Feb 11, 2018

@amihaiemil just solved some conflicts with an updated master branch, please check

@amihaiemil

This comment has been minimized.

Contributor

amihaiemil commented Feb 11, 2018

@yegor256 don't forget about this one. Clock is ticking

@0crat

This comment has been minimized.

Collaborator

0crat commented Feb 11, 2018

@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

This comment has been minimized.

Contributor

amihaiemil commented Feb 12, 2018

@yegor256 ping

@yegor256

This comment has been minimized.

Owner

yegor256 commented Feb 12, 2018

@rultor merge

@rultor

This comment has been minimized.

Collaborator

rultor commented Feb 12, 2018

@rultor merge

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

@rultor

This comment has been minimized.

Collaborator

rultor commented Feb 12, 2018

@rultor merge

@llorllale @yegor256 Oops, I failed. You can see the full log here (spent 12min)

Downloading: http://repo.maven.apache.org/maven2/org/apache/maven/maven-plugin-api/3.0/maven-plugin-api-3.0.jar
Downloading: http://repo.maven.apache.org/maven2/org/apache/maven/maven-model/3.0/maven-model-3.0.jar
Downloading: http://repo.maven.apache.org/maven2/org/codehaus/plexus/plexus-utils/2.0.4/plexus-utils-2.0.4.jar
Downloading: http://repo.maven.apache.org/maven2/org/apache/maven/maven-artifact/3.0/maven-artifact-3.0.jar
Downloading: http://repo.maven.apache.org/maven2/org/sonatype/sisu/sisu-inject-plexus/1.4.2/sisu-inject-plexus-1.4.2.jar

Downloaded: http://repo.maven.apache.org/maven2/org/apache/maven/maven-plugin-api/3.0/maven-plugin-api-3.0.jar (0 B at 0.0 KB/sec)
Downloading: http://repo.maven.apache.org/maven2/org/codehaus/plexus/plexus-classworlds/2.2.3/plexus-classworlds-2.2.3.jar


Downloaded: http://repo.maven.apache.org/maven2/org/apache/maven/maven-model/3.0/maven-model-3.0.jar (0 B at 0.0 KB/sec)
Downloaded: http://repo.maven.apache.org/maven2/org/apache/maven/maven-artifact/3.0/maven-artifact-3.0.jar (0 B at 0.0 KB/sec)
Downloading: http://repo.maven.apache.org/maven2/org/sonatype/sisu/sisu-inject-bean/1.4.2/sisu-inject-bean-1.4.2.jar


Downloading: http://repo.maven.apache.org/maven2/org/sonatype/sisu/sisu-guice/2.1.7/sisu-guice-2.1.7-noaop.jar
Downloaded: http://repo.maven.apache.org/maven2/org/codehaus/plexus/plexus-utils/2.0.4/plexus-utils-2.0.4.jar (0 B at 0.0 KB/sec)
Downloaded: http://repo.maven.apache.org/maven2/org/codehaus/plexus/plexus-classworlds/2.2.3/plexus-classworlds-2.2.3.jar (0 B at 0.0 KB/sec)

Downloading: http://repo.maven.apache.org/maven2/org/apache/maven/shared/maven-shared-utils/3.0.0/maven-shared-utils-3.0.0.jar
Downloaded: http://repo.maven.apache.org/maven2/org/sonatype/sisu/sisu-inject-plexus/1.4.2/sisu-inject-plexus-1.4.2.jar (0 B at 0.0 KB/sec)


Downloaded: http://repo.maven.apache.org/maven2/org/sonatype/sisu/sisu-guice/2.1.7/sisu-guice-2.1.7-noaop.jar (0 B at 0.0 KB/sec)

Downloaded: http://repo.maven.apache.org/maven2/org/apache/maven/shared/maven-shared-utils/3.0.0/maven-shared-utils-3.0.0.jar (0 B at 0.0 KB/sec)
Downloaded: http://repo.maven.apache.org/maven2/org/sonatype/sisu/sisu-inject-bean/1.4.2/sisu-inject-bean-1.4.2.jar (0 B at 0.0 KB/sec)
[INFO] Deleting /home/r/repo/target
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 1.100 s
[INFO] Finished at: 2018-02-12T12:19:42+00:00
[INFO] Final Memory: 13M/361M
[INFO] ------------------------------------------------------------------------
++ pwd
+ pdd --source=/home/r/repo --verbose --file=/dev/null
Found 7 lines in /home/r/repo/.pdd
My version is 0.20
Ruby version is 2.2.6 at x86_64-linux-gnu
Reading /home/r/repo
Excluding target/**/*
Excluding src/site/resources/**/*
133 file(s) found, 231 excluded
/home/r/repo/papers/counsell06.pdf is a binary file (255738 bytes)
/home/r/repo/papers/aman04.pdf is a binary file (144634 bytes)
/home/r/repo/papers/sellers96.pdf is a binary file (167522 bytes)
/home/r/repo/papers/badri08.pdf is a binary file (289106 bytes)
/home/r/repo/papers/etzkorn00.pdf is a binary file (553326 bytes)
/home/r/repo/papers/chidamber94.pdf is a binary file (1661552 bytes)
/home/r/repo/papers/izadkhah17.pdf is a binary file (265225 bytes)
/home/r/repo/papers/dallal07.pdf is a binary file (1821388 bytes)
/home/r/repo/papers/bansiya99.pdf is a binary file (96858 bytes)
/home/r/repo/papers/fernandez06.pdf is a binary file (155707 bytes)
/home/r/repo/papers/wasiq01.pdf is a binary file (4134387 bytes)
Reading .0pdd.yml...
Reading .github/ISSUE_TEMPLATE.md...
Reading .github/PULL_REQUEST_TEMPLATE.md...
Reading .travis.yml...
Reading LICENSE.txt...
Reading .pdd...
Reading .rultor.yml...
Reading README.md...
Reading papers/henderson96.pdf...
\u001b[31mERROR\u001b[0m: papers/henderson96.pdf; puzzle at line #2; invalid byte sequence in UTF-8. If you can't understand the cause of this issue or you don't know how to fix it, please submit a GitHub issue, we will try to help you: https://github.com/yegor256/pdd/issues. This tool is still in its beta version and we will appreciate your feedback. Here is where you can find more documentation: https://github.com/yegor256/pdd/blob/master/README.md.
Exit code is 1
container 212f8ae713644b0878fca9f56705edcd11020e542aec796d9d4dfb83debb8778 is dead
Mon Feb 12 13:20:11 CET 2018

@llorllale

This comment has been minimized.

Contributor

llorllale commented Feb 12, 2018

@yegor256 @amihaiemil pdd failed because of a pdf. What do do here?

@amihaiemil

This comment has been minimized.

Contributor

amihaiemil commented Feb 12, 2018

@yegor256 can you try to merge again?

@yegor256

This comment has been minimized.

Owner

yegor256 commented Feb 12, 2018

@rultor merge

@rultor

This comment has been minimized.

Collaborator

rultor commented Feb 12, 2018

@rultor merge

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

@rultor

This comment has been minimized.

Collaborator

rultor commented Feb 12, 2018

@rultor merge

@llorllale @yegor256 Oops, I failed. You can see the full log here (spent 12min)

Downloading: http://repo.maven.apache.org/maven2/org/apache/maven/maven-plugin-api/3.0/maven-plugin-api-3.0.jar
Downloading: http://repo.maven.apache.org/maven2/org/apache/maven/maven-model/3.0/maven-model-3.0.jar
Downloading: http://repo.maven.apache.org/maven2/org/codehaus/plexus/plexus-utils/2.0.4/plexus-utils-2.0.4.jar
Downloading: http://repo.maven.apache.org/maven2/org/apache/maven/maven-artifact/3.0/maven-artifact-3.0.jar
Downloading: http://repo.maven.apache.org/maven2/org/sonatype/sisu/sisu-inject-plexus/1.4.2/sisu-inject-plexus-1.4.2.jar

Downloaded: http://repo.maven.apache.org/maven2/org/apache/maven/maven-plugin-api/3.0/maven-plugin-api-3.0.jar (0 B at 0.0 KB/sec)
Downloading: http://repo.maven.apache.org/maven2/org/codehaus/plexus/plexus-classworlds/2.2.3/plexus-classworlds-2.2.3.jar

Downloaded: http://repo.maven.apache.org/maven2/org/apache/maven/maven-model/3.0/maven-model-3.0.jar (0 B at 0.0 KB/sec)
Downloading: http://repo.maven.apache.org/maven2/org/sonatype/sisu/sisu-inject-bean/1.4.2/sisu-inject-bean-1.4.2.jar

Downloaded: http://repo.maven.apache.org/maven2/org/codehaus/plexus/plexus-classworlds/2.2.3/plexus-classworlds-2.2.3.jar (0 B at 0.0 KB/sec)


Downloading: http://repo.maven.apache.org/maven2/org/sonatype/sisu/sisu-guice/2.1.7/sisu-guice-2.1.7-noaop.jar
Downloaded: http://repo.maven.apache.org/maven2/org/codehaus/plexus/plexus-utils/2.0.4/plexus-utils-2.0.4.jar (0 B at 0.0 KB/sec)
Downloading: http://repo.maven.apache.org/maven2/org/apache/maven/shared/maven-shared-utils/3.0.0/maven-shared-utils-3.0.0.jar
Downloaded: http://repo.maven.apache.org/maven2/org/apache/maven/maven-artifact/3.0/maven-artifact-3.0.jar (0 B at 0.0 KB/sec)

Downloaded: http://repo.maven.apache.org/maven2/org/sonatype/sisu/sisu-inject-plexus/1.4.2/sisu-inject-plexus-1.4.2.jar (0 B at 0.0 KB/sec)


Downloaded: http://repo.maven.apache.org/maven2/org/sonatype/sisu/sisu-inject-bean/1.4.2/sisu-inject-bean-1.4.2.jar (0 B at 0.0 KB/sec)
Downloaded: http://repo.maven.apache.org/maven2/org/sonatype/sisu/sisu-guice/2.1.7/sisu-guice-2.1.7-noaop.jar (0 B at 0.0 KB/sec)

Downloaded: http://repo.maven.apache.org/maven2/org/apache/maven/shared/maven-shared-utils/3.0.0/maven-shared-utils-3.0.0.jar (0 B at 0.0 KB/sec)
[INFO] Deleting /home/r/repo/target
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 1.150 s
[INFO] Finished at: 2018-02-12T13:40:34+00:00
[INFO] Final Memory: 13M/364M
[INFO] ------------------------------------------------------------------------
++ pwd
+ pdd --source=/home/r/repo --verbose --file=/dev/null
Found 7 lines in /home/r/repo/.pdd
My version is 0.20
Ruby version is 2.2.6 at x86_64-linux-gnu
Reading /home/r/repo
Excluding target/**/*
Excluding src/site/resources/**/*
133 file(s) found, 231 excluded
/home/r/repo/papers/counsell06.pdf is a binary file (255738 bytes)
/home/r/repo/papers/aman04.pdf is a binary file (144634 bytes)
/home/r/repo/papers/sellers96.pdf is a binary file (167522 bytes)
/home/r/repo/papers/badri08.pdf is a binary file (289106 bytes)
/home/r/repo/papers/etzkorn00.pdf is a binary file (553326 bytes)
/home/r/repo/papers/chidamber94.pdf is a binary file (1661552 bytes)
/home/r/repo/papers/izadkhah17.pdf is a binary file (265225 bytes)
/home/r/repo/papers/dallal07.pdf is a binary file (1821388 bytes)
/home/r/repo/papers/bansiya99.pdf is a binary file (96858 bytes)
/home/r/repo/papers/fernandez06.pdf is a binary file (155707 bytes)
/home/r/repo/papers/wasiq01.pdf is a binary file (4134387 bytes)
Reading .0pdd.yml...
Reading .github/ISSUE_TEMPLATE.md...
Reading .github/PULL_REQUEST_TEMPLATE.md...
Reading .travis.yml...
Reading LICENSE.txt...
Reading .pdd...
Reading .rultor.yml...
Reading README.md...
Reading papers/henderson96.pdf...
\u001b[31mERROR\u001b[0m: papers/henderson96.pdf; puzzle at line #2; invalid byte sequence in UTF-8. If you can't understand the cause of this issue or you don't know how to fix it, please submit a GitHub issue, we will try to help you: https://github.com/yegor256/pdd/issues. This tool is still in its beta version and we will appreciate your feedback. Here is where you can find more documentation: https://github.com/yegor256/pdd/blob/master/README.md.
Exit code is 1
container cc05eb2ba4e332d36df646c17e471b954aa69dc5eedcc29dc88feff62e88910b is dead
Mon Feb 12 14:42:06 CET 2018

@llorllale

This comment has been minimized.

Contributor

llorllale commented Feb 12, 2018

@amihaiemil @yegor256 the build is failing on this file: papers/henderson96.pdf. As you can see, I did not touch this file in this PR. What can we do here?

@amihaiemil

This comment has been minimized.

Contributor

amihaiemil commented Feb 12, 2018

@llorllale I think you should open a bug about it. Probably someone merged a change in that file, in some other PR. Then, ask 0crat to add an impediment to your task so you don't lose it. However, I have no idea what to do about this PR, as far as I know, PRs cannot have impediments...

@llorllale

This comment has been minimized.

Contributor

llorllale commented Feb 12, 2018

@amihaiemil let's check now - I believe Yegor fixed the issue

@llorllale

This comment has been minimized.

Contributor

llorllale commented Feb 12, 2018

@amihaiemil @yegor256 opened #172 for a new build issue

@yegor256

This comment has been minimized.

Owner

yegor256 commented Feb 12, 2018

@rultor merge

@rultor

This comment has been minimized.

Collaborator

rultor commented Feb 12, 2018

@rultor merge

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

@rultor rultor merged commit 6a7aa75 into yegor256:master Feb 12, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@rultor

This comment has been minimized.

Collaborator

rultor commented Feb 12, 2018

@rultor merge

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

@0crat

This comment has been minimized.

Collaborator

0crat commented Feb 12, 2018

Order was successfully finished: +15 points just awarded to @amihaiemil/z, total is +525

@0crat

This comment has been minimized.

Collaborator

0crat commented Feb 12, 2018

Payment to ARC for a closed pull request, as in §28: +15 points just awarded to @yegor256/z, total is +9810

@0crat

This comment has been minimized.

Collaborator

0crat commented Feb 12, 2018

The job #153 is now out of scope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment