- 
                Notifications
    
You must be signed in to change notification settings  - Fork 7.6k
 
2.x: fix concatMapEager should accept 0 for prefetch #5189
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
Conversation
| 
           I believe a 0 breaks the internals of the operator and -1 has the completely opposite effect, taking more memory.  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The verifyPositive should be restored.
Optionally, you could add the ", positive" to the javadoc changes.
| public final <R> Flowable<R> concatMapEager(Function<? super T, ? extends Publisher<? extends R>> mapper, | ||
| int maxConcurrency, int prefetch) { | ||
| ObjectHelper.verifyPositive(maxConcurrency, "maxConcurrency"); | ||
| ObjectHelper.verifyPositive(prefetch, "prefetch"); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the validations were right, you shouldn't call with non-positive value.
| int maxConcurrency, int prefetch) { | ||
| ObjectHelper.requireNonNull(mapper, "mapper is null"); | ||
| ObjectHelper.verifyPositive(maxConcurrency, "maxConcurrency"); | ||
| ObjectHelper.verifyPositive(prefetch, "prefetch"); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the validations were right, you shouldn't call with non-positive value.
| * eagerly concatenated | ||
| * @param maxConcurrency the maximum number of concurrent subscribed Publishers | ||
| * @param prefetch hints about the number of expected source sequence values | ||
| * @param prefetch hints about the number of expected values from each inner Publisher | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be reinforced:
hints about the number of expected values from each inner Publisher, must be positive
| 
           I am sorry with the wrong fix.  | 
    
          
 Yes. Please modify only the javadoc.  | 
    
9e5b703    to
    00bfea6      
    Compare
  
    | 
           Thank you! Also, I think that the test case name is wrong, so I modifed it.  | 
    
          Codecov Report
 @@             Coverage Diff              @@
##                2.x    #5189      +/-   ##
============================================
- Coverage     96.02%   95.97%   -0.06%     
  Complexity     5670     5670              
============================================
  Files           621      621              
  Lines         40581    40581              
  Branches       5620     5620              
============================================
- Hits          38969    38946      -23     
- Misses          640      661      +21     
- Partials        972      974       +2
 Continue to review full report at Codecov. 
  | 
    
| 
           Thanks for the contribution!  | 
    
Fixes a bug that Flowable.concatMapEager(mapper , maxConcurrency , prefetch) and Observable.concatMapEager(mapper , maxConcurrency , prefetch) operators will not accept 0 and negative numbers.
Reported in #5185.