Make ErrorCollector#checkSucceeds() generic #1013
Conversation
|
Thanks for the fix. Could you please update the tests for |
|
There you go =) I rolled back the change to the checkThat-methods, since they work just fine without the lower bound. It would only be a problem if they used the type parameter T somewhere else, e.g. as a return type. |
| @@ -73,7 +73,7 @@ public Object call() throws Exception { | |||
| * Execution continues, but the test will fail at the end if | |||
| * {@code callable} threw an exception. | |||
| */ | |||
| public Object checkSucceeds(Callable<Object> callable) { | |||
| public Object checkSucceeds(Callable<?> callable) { | |||
There was a problem hiding this comment.
I think a better signature would be:
public <V> V checkSucceeds(Callable<V> callable)
If this is the only change (other than the tests) could you update the commit message to be someone more specific? Perhaps "Make ErrorCollector.checkSucceeds() generic"
oehme
changed the title
| @@ -73,7 +73,7 @@ public Object call() throws Exception { | |||
| * Execution continues, but the test will fail at the end if | |||
| * {@code callable} threw an exception. | |||
| */ | |||
| public Object checkSucceeds(Callable<Object> callable) { | |||
| public <T> T checkSucceeds(Callable<? extends T> callable) { | |||
There was a problem hiding this comment.
This should really be:
public <T> T checkSucceeds(Callable<T> callable) {
.. because of the return type of Callable.call(). Stated another way, if you pass in a Callable<Integer> then the return type should be a Integer, but the signature you have would mean "return some super type of Integer"
|
@oehme Sorry for not responding earlier. Github didn't notify us that you had pushed a new commit. I think you need to ping the thread for us to be notified. |
|
Oh I see. Well no problem, I'm glad I could help =) 2014-11-27 5:36 GMT+01:00 Kevin Cooney [email protected]:
|
|
@oehme thanks for.helping! Any chance you could fix the generics on |
|
Oh sorry I didn't see that comment. You're right, that upper bound is not necessary, I'll fix that. |
|
I fixed the signature on my side and merged this pull. Thanks! |
This brings the signatures in line with others like Assert.assertThat.
Especially the checkSucceeds would be hardly usable in languages with target typing (Java8/Xtend) otherwise.