Codecov Report

Merging #523 into master will decrease coverage by 7.48%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #523      +/-   ##
==========================================
- Coverage    76.9%   69.42%   -7.49%     
==========================================
  Files          63        1      -62     
  Lines        5582      278    -5304     
  Branches      890        0     -890     
==========================================
- Hits         4293      193    -4100     
+ Misses        998       85     -913     
+ Partials      291        0     -291

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 545c549...e40995c. Read the comment docs.

For future reference, the only place where a general "catch (Exception E)" and subsequent Exceptions.SetError() is done without throwing away the outermost exception is in TypeMethod.Invoke.

@rickardraysearch can you clarify about that TypeMethod.Invoke? Is this an issue or just a note?

@denfromufa , I only wanted to note that the usage is not completely consistent across all invocations of Exceptions.SetError(). When we revisit how to do the SetErrors, I want to remember this without having to find it out again.

@filmor @vmuriart please review!

What happens if you call next() after the exception is thrown? Might be worth adding a test for it to make sure it has expected behavior.

Other than adding that test, ok to merge

What happens if you call next() after the exception is thrown? Might be worth adding a test for it to make sure it has expected behavior.

Other than adding that test, ok to merge

@vmuriart did you mean calling this part twice?

with pytest.raises(OverflowException) as cm:
    next(val)

@vmuriart or did you mean to verify this behavior?

https://stackoverflow.com/a/11366139/2230844

both :).

@rickardraysearch can you please address the suggestions from @vmuriart and then we can merge this!

We just ran into the same issue that this pull request is going to fix. Is there any particular reason this was never merged? If there is further work to be done we would be happy to help.

@ArvidJB i just updated this branch with latest master and CI tests look good. You can add 2 additional unit tests suggested by @vmuriart but personally I approved this PR. If one more core developers approves, then we can merge this. @filmor had a question and that probably can be addressed in another PR.

@denfromufa / @vmuriart I think this boils down to adding the following lines

    # after exception is thrown iterator is no longer valid
    with pytest.raises(StopIteration):
        next(val)

to test_iteration_innerexception. Should I create a new pull request for that or would you be willing to add those lines to @rickardraysearch 's request?
I tested @rickardraysearch 's changes and they work as intended.

@ArvidJB please fork @rickardraysearch branch and make a new pull request with your changes. If you PR looks good, then we can close this PR.

@denfromufa I just submitted #693 with the enhanced testing

Merged as #693.