#417 #423
#417 #423
Conversation
|
@0crat assign @fanifieiev |
|
@paulodamaso This pull request #423 is assigned to @fanifieiev/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 |
| @Disabled | ||
| public void methodMethodCalls() throws Exception { | ||
| @CsvFileSource(resources = "/org/jpeek/calculus/java/lcom4-params.csv") | ||
| public void methodMethodCalls(final String file, final String value) throws Exception { |
There was a problem hiding this comment.
@HDouss I know that is not your change, but please change the name of the method to a meaningful one
| @Disabled | ||
| public void methodMethodCalls() throws Exception { | ||
| @CsvFileSource(resources = "/org/jpeek/calculus/java/lcom4-params.csv") | ||
| public void methodMethodCalls(final String file, final String value) throws Exception { | ||
| final XML result = new Lcom4().node( |
There was a problem hiding this comment.
@HDouss I think Lcom4 should have a constructor or several constructors to accept parameters and the only its interfaced method node should be parameterless.
I mean, Calculus should have:
public interface Calculus {
XML node()
throws IOException;
}
and Lcom4 should have a cosntructor to accept the values.
So, I suggest leaving a todo
| final XML result = new Lcom4().node( | ||
| "", new HashMap<>(0), new Skeleton( | ||
| new FakeBase("MethodMethodCalls") | ||
| new FakeBase(file) | ||
| ).xml() | ||
| ); | ||
| new Assertion<>( | ||
| "Must create LCOM4 value", | ||
| result.xpath("/metric/app/package/class/@value").get(0), |
There was a problem hiding this comment.
@HDouss The Cactoos has ItemAt scalar, so you could use it like an elegant way:
new ItemAt(0, result.xpath("/metric/app/package/class/@value"))
and test scalars
|
@fanifieiev Actually all your 3 comments are not related to the issue or the PR. I fixed comment 1&3. The comment 2 (about node interface method): I am not sure this is a good idea + this is a rather big refactoring as it will affect xsl calculus as well, along their usages: metrics classes, App class, Test classes... Please submit a new issue so the ARC could state if it is a good idea (also because it is unrelated to that issue). |
@HDouss I know it might not be related to this PR, but, reviewing even the old code seen in PRs, helps the reviewer detect potential design problems and ask DEV to leave a todo. |
|
@paulodamaso ping |
@fanifieiev In this case I think that it's better that you open an issue and get the payment for bug report. Asking the DEV to leave a puzzle don't earn you the payment for this bug. Usually we ask the performer to leave puzzles when the budget for the PR was already spent (30 min) but there are some features still missing. |
@paulodamaso OK))) |
| result.xpath("/metric/app/package/class/@value").get(0), | ||
| new IsEqual<>("1") | ||
| new ItemAt<>(0, result.xpath("/metric/app/package/class/@value")).value(), | ||
| new IsEqual<>(value) |
|
@fanifieiev Please review again |
|
@paulodamaso ping |
|
@HDouss Thank you |
|
@rultor merge |
@paulodamaso OK, I'll try to merge now. You can check the progress of the merge here |
@HDouss @paulodamaso Oops, I failed. You can see the full log here (spent 9min) |
|
@paulodamaso seems we still have problems with rultor |
|
@HDouss Yes, I'm trying to fix it since last week. |
|
@rultor merge |
@paulodamaso OK, I'll try to merge now. You can check the progress of the merge here |
@HDouss @paulodamaso Oops, I failed. You can see the full log here (spent 2min) |
|
@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 22min) |
|
Job was finished in 27 hours, bonus for fast delivery is possible (see §36) |
Resolving issue #417
Added LCOM4 test cases.
(fixed expected value for MethodMethodCalls)