1.x: apply fixes based on PMD suggestions #4091
Conversation
|
Travis doesn't pick up this one for some reason. Reopening. |
Current coverage is 80.16%@@ 1.x #4091 diff @@
==========================================
Files 259 257 -2
Lines 16821 16771 -50
Methods 0 0
Messages 0 0
Branches 2554 2541 -13
==========================================
- Hits 13500 13445 -55
- Misses 2408 2416 +8
+ Partials 913 910 -3
|
|
/cc @hzsweers @artem-zinnatullin Let me know if you want to review this and need some time. |
|
I'll review it in 24 hours if you don't mind |
|
Sure. I'll return to 2.x till then. |
| @@ -151,9 +151,6 @@ public void remove() { | |||
| private final BlockingQueue<Notification<? extends T>> buf = new ArrayBlockingQueue<Notification<? extends T>>(1); | |||
| final AtomicInteger waiting = new AtomicInteger(); | |||
|
|
|||
| NextObserver() { | |||
There was a problem hiding this comment.
Removing this adds a synthetic accessor method.
|
Some of these I really like. Some I don't. Documenting empty methods seems like pure noise. |
|
Updated. |
|
@JakeWharton we can iterate on the ruleset after this gets merged. |
|
👍 |
|
Okay then. If you don't mind, let's have the comments in this PR as is and have a separate PR disable the rule. |
| /** | ||
| * By default, signal a MissingBackressureException due to lack of requests. | ||
| */ | ||
| public static final BackpressureOverflow.Strategy ON_OVERFLOW_DEFAULT = Error.INSTANCE; |
There was a problem hiding this comment.
Would be nice to move this below all other strategies and change assignment to ON_OVERFLOW_DEFAULT = ON_OVERFLOW_ERROR so it'll be more clear that default strategy is same as ON_OVERFLOW_ERROR
There was a problem hiding this comment.
Maybe, how about in a separate PR after this?
|
👍 Sound good! |
| @@ -377,7 +379,7 @@ public static Completable create(CompletableOnSubscribe onSubscribe) { | |||
| requireNonNull(onSubscribe); | |||
| try { | |||
| return new Completable(onSubscribe); | |||
| } catch (NullPointerException ex) { | |||
| } catch (NullPointerException ex) { // NOPMD by akarnokd on 2016.06.23. 10:54 | |||
There was a problem hiding this comment.
will be great to avoid such comments by tweaking pmd rules
There was a problem hiding this comment.
or at least remove "by akarnokd on …", we have git history for that
| @@ -2589,7 +2593,7 @@ public void call(SingleSubscriber<? super T> singleSubscriber) { | |||
| */ | |||
| @SuppressWarnings("unchecked") | |||
| static <T> Single<? extends T>[] iterableToArray(final Iterable<? extends Single<? extends T>> singlesIterable) { | |||
| final Single<? extends T>[] singlesArray; | |||
| Single<? extends T>[] singlesArray; | |||
There was a problem hiding this comment.
PMD complained about final variable?
|
Updated: removed my name, swapped the BackpressureStategy constants, disabled empty method/constructor/catch block rules. |
|
👍 |
| @@ -2410,10 +2412,12 @@ public void onNext(T t) { | |||
| Observer<T> observer = new Observer<T>() { | |||
| @Override | |||
| public void onCompleted() { | |||
| // deliberately ignored | |||
| } | |||
|
|
|||
| @Override | |||
| public void onError(Throwable e) { | |||
There was a problem hiding this comment.
Not sure if this is covered in PMD, but usually like to also name the param ignored. Same for the others. I suppose it's kind of orthogonal to the comment here though, which is that the method itself is deliberately unimplemented. I'd recommend specifying that it's unimplemented rather than "ignored", since "ignored" usually is in reference to the param(s)
|
Some minor nits, LGTM 👍 |
|
Thanks @hzsweers! |
Some notable changes:
while (e != r)patternExceptions.propagate