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

Changed logic of iterator termination #330

Merged
merged 2 commits into from
Jul 13, 2019
Merged

Changed logic of iterator termination #330

merged 2 commits into from
Jul 13, 2019

Conversation

ziflex
Copy link
Member

@ziflex ziflex commented Jul 11, 2019

Originally, if iterator returned NONE value, it meant, that this is the end of iteration and there are no values.
This approach has one big flaw - it does not work in scenarios, when an underlying source contains NONE values.
This PR changes it. Now, whenever iterator gets exhausted it returns core.NoMoreData error and all consumers should check output on this error.

Before:

for {
   value, key, err := iterator.Next()

   if value == values.None {
       break
   }
}

Now:

for {
   value, key, err := iterator.Next()

   if err != nil {
       if core.IsNoMoreData(err) {
          break
       }

      return err
   }
}

@ziflex ziflex requested a review from 3timeslazy July 11, 2019 21:40
@codecov
Copy link

codecov bot commented Jul 11, 2019

Codecov Report

Merging #330 into master will decrease coverage by 2.1%.
The diff coverage is 55.6%.

@@           Coverage Diff            @@
##           master    #330     +/-   ##
========================================
- Coverage    39.7%   37.5%   -2.1%     
========================================
  Files         216     216             
  Lines        8578    9146    +568     
========================================
+ Hits         3404    3434     +30     
- Misses       4819    5355    +536     
- Partials      355     357      +2

@ziflex ziflex merged commit 9756f0d into master Jul 13, 2019
@ziflex ziflex deleted the refactoring/iterator branch July 13, 2019 17:39
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.

2 participants