Skip to content
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

improve generics of promise #158

Merged
merged 3 commits into from Dec 17, 2017

Conversation

sclassen
Copy link
Contributor

@sclassen sclassen commented Dec 16, 2017

resolves #154

Note: This is a breaking API change
Never the less I think this will improve the experience of using jdeferred as the typing becomes more strict and therefore reduces possible runtime exceptions (such as ClassCastException)

On the same time by using "? super" and "? extends" the typing becomes relaxed as we now can do the following

Promise<Integer, Throwable, Void> integerPromise = createIntegerPromise();
Promise<Float, Throwable, Void> floatPromise = createFloatPromise();
integerPromise.then(Helper::numberLogger)
floatPromise.then(Helper::numberLogger)

where

public class Helper {
    public void numberLogger(Number n) {
        /* log number */
    }
}

The Java Stream API also uses "? super" and "? extends" to allow maximal flexibility


This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage remained the same at 55.912% when pulling b7ef67d on sclassen:154_improveGenericsOfPromise into ccd35d4 on jdeferred:2.x.

@aalmiray
Copy link
Contributor

Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


subprojects/jdeferred-core/src/test/java/org/jdeferred/impl/PipedPromiseTest.java, line 40 at r1 (raw file):

		};
		
		deferredManager.when(task).then(new DonePipe<Integer, Integer, Throwable, Void>() {

just curious, why the change from String to Throwable?


Comments from Reviewable

@sclassen
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


subprojects/jdeferred-core/src/test/java/org/jdeferred/impl/PipedPromiseTest.java, line 40 at r1 (raw file):

Previously, aalmiray (Andres Almiray) wrote…

just curious, why the change from String to Throwable?

You mean from Void to Throwable.
This is do to the stricter typing enforced by the changes. The deferredManager.when(task) returns a Promise<Integer, Throwable, Void> therefore the DonePipe also needs to return a Promise<D_OUT, Throwable, Void>. D_OUT is Integer in this the concrete implementation.
This is one of the cases where the API is breaking but it is more stringent because if the DonePipe would return Promise<D_OUT, F_OUT, Void> where F_OUT != F then there are two possible rejected types.

  1. Throwable if the original promise is rejected
  2. F_OUT if the promise returned from the DonePipe.pipeDone() is rejected

This may be OK for untyped languages such as Javascript but for Java we should prevent this cases


Comments from Reviewable

@aalmiray
Copy link
Contributor

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


subprojects/jdeferred-core/src/test/java/org/jdeferred/impl/PipedPromiseTest.java, line 40 at r1 (raw file):

Previously, sclassen wrote…

You mean from Void to Throwable.
This is do to the stricter typing enforced by the changes. The deferredManager.when(task) returns a Promise<Integer, Throwable, Void> therefore the DonePipe also needs to return a Promise<D_OUT, Throwable, Void>. D_OUT is Integer in this the concrete implementation.
This is one of the cases where the API is breaking but it is more stringent because if the DonePipe would return Promise<D_OUT, F_OUT, Void> where F_OUT != F then there are two possible rejected types.

  1. Throwable if the original promise is rejected
  2. F_OUT if the promise returned from the DonePipe.pipeDone() is rejected

This may be OK for untyped languages such as Javascript but for Java we should prevent this cases

Not really, no. I meant the change made to testDoneRewireToFail where the failure used to be of type String but now is of type Throwable. The test still works anyway.


Comments from Reviewable

@sclassen
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


subprojects/jdeferred-core/src/test/java/org/jdeferred/impl/PipedPromiseTest.java, line 40 at r1 (raw file):

Previously, aalmiray (Andres Almiray) wrote…

Not really, no. I meant the change made to testDoneRewireToFail where the failure used to be of type String but now is of type Throwable. The test still works anyway.

Same reasoning. The deferredManager.when(new Callable<Integer>() {...}) returns a Promise<Integer, Throwable, Void> and if we only specify a DonePipe and no FailPipe then F_OUT must be the same as F.


Comments from Reviewable

@aalmiray
Copy link
Contributor

Review status: all files reviewed at latest revision, 1 unresolved discussion.


subprojects/jdeferred-core/src/test/java/org/jdeferred/impl/PipedPromiseTest.java, line 40 at r1 (raw file):

Previously, sclassen wrote…

Same reasoning. The deferredManager.when(new Callable<Integer>() {...}) returns a Promise<Integer, Throwable, Void> and if we only specify a DonePipe and no FailPipe then F_OUT must be the same as F.

ah got it :-)


Comments from Reviewable

@aalmiray
Copy link
Contributor

:lgtm: @saturnism we'll have to merge with caution due to #156. In other words the fix for #156 should go last


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@saturnism
Copy link
Member

@aalmiray agreed; let's close out the last of the PRs before #156.

@saturnism
Copy link
Member

Reviewed 7 of 8 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@saturnism
Copy link
Member

:lgtm:


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@saturnism saturnism merged commit 858126e into jdeferred:2.x Dec 17, 2017
@sclassen sclassen deleted the 154_improveGenericsOfPromise branch January 7, 2018 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants