Make CommitsComparison.files() return typed objects instead of raw JSON #1120
Conversation
|
|
||
| @Override | ||
| public FileChange next() { | ||
| return new RtFileChange(this.iterator.next()); |
There was a problem hiding this comment.
FindBugs currently gives a false-positive warning here:
[WARNING] FindBugs: L B It: com.jcabi.github.RtCommitsComparison$FileChangesIterator.next() can't throw NoSuchElementException At RtCommitsComparison.java:[line 134]
I don't know how to tell qulice to suppress this.
More info on this particular warning: http://findbugs.sourceforge.net/bugDescriptions.html#IT_NO_SUCH_ELEMENT
Adding an explicit if (!this.iterator.hasNext()) { throw new NoSuchElementException("No more file changes!"); } didn't make the warning go away.
There was a problem hiding this comment.
@cvrebert You should be able to use the SuppressFBWarnings annotation: @SuppressFBWarnings("IT_NO_SUCH_ELEMENT")
There was a problem hiding this comment.
@carlosmiranda Done. Thanks!
|
@cvrebert Thanks, I will find someone to review this PR soon |
|
@carlosmiranda please review this PR |
| @EqualsAndHashCode(of = { "list" }) | ||
| @Loggable(Loggable.DEBUG) | ||
| @ToString | ||
| private static final class FileChangesIterable |
There was a problem hiding this comment.
@cvrebert Let's name this class, simply, FileChanges.
There was a problem hiding this comment.
@carlosmiranda Done.
|
@cvrebert Good branch, just some minor comments. This diff is huge, however. You could have left some of it (e.g. the IT cases) as puzzles instead of implementing everything in one go. Makes things easier to review (and we typically have task budgets of 30min). No need for now, but do keep it in mind next time. |
|
@carlosmiranda I've made all of your requested changes. |
|
@rultor merge please |
@carlosmiranda Thanks for your request. @yegor256 Please confirm this. |
|
@cvrebert Please edit PR title to something more descriptive. |
cvrebert
changed the title
|
@carlosmiranda Done. |
| @@ -145,6 +145,11 @@ | |||
| <artifactId>validation-api</artifactId> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>com.google.code.findbugs</groupId> | |||
| <artifactId>annotations</artifactId> | |||
| <version>3.0.0</version> | |||
There was a problem hiding this comment.
@cvrebert can we find some other workaround? I really don't like the idea of adding this dependency to the "compile" scope
There was a problem hiding this comment.
@yegor256 Does the FindBugs Qulice Maven plugin provide a way to configure exclusions?
There was a problem hiding this comment.
@cvrebert check this: http://stackoverflow.com/questions/30702059/how-to-ignore-or-fix-the-duplicate-classes-warning-in-qulice/30814032#30814032 you can completely suppress findbugs, just by using <exclusion>findbugs:.*</exclusion>, but I don't think it's a good idea.. maybe let's do what findbugs is asking us to do?
There was a problem hiding this comment.
maybe let's do what findbugs is asking us to do?
@yegor256 Tried that, still warns. It's a genuine false positive; see #1120 (comment)
There was a problem hiding this comment.
FindBugs itself allows for more granular suppression via XML filter files (http://findbugs.sourceforge.net/manual/filter.html), but I don't think Qulice currently supports this.
|
@cvrebert see my comment above |
|
@rultor merge |
|
@rultor merge |
|
@rultor merge |
@yegor256 Oops, I failed. You can see the full log here (spent 3min) |
|
@yegor256 Hmm, there's some problem with my integration test. Will investigate. |
… JsonArray; fixes #1082
|
@yegor256 Fixed. I had |
|
@cvrebert Please edit your comments so that it addresses somebody, e.g.
Otherwise QA will be hounding us. :) |
|
@rultor merge |
@yegor256 Oops, I failed. You can see the full log here (spent 6min) |
|
@cvrebert Failures are unrelated: |
|
@rultor merge |
|
@cvrebert Please, always address your messages. See www.teamed.io/qa.html. |
|
@carlosmiranda Do my comments affect your compensation as code-reviewer? |
|
@cvrebert QA will ask you to correct them before I get paid so I might as well ask you to fix them now. |
|
@carlosmiranda I have never seen QA on jcabi-github. |
|
@rultor merge |
|
@rultor merge pls |
@yegor256 Oops, I failed. You can see the full log here (spent 4min) |
|
@carlosmiranda Error is still unrelated: |
|
@rultor merge |
|
@rultor try to merge |
|
@carlosmiranda 22 mins sent to your balance (ID |
|
@rultor deploy |
Fixes #1082 by adding a
FileChangeinterface and havingCommitsComparison.files()return anIterator<FileChange>instead of a rawJsonArray.