Skip to content

py: Fix errors are suppressed in generator comprehensions #38

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Nov 29, 2018

Conversation

corona10
Copy link
Collaborator

Sequence.Iterate does not handle exceptions.
Iterate() should return error except StopIteration.

Fixes: #32

@corona10 corona10 requested a review from ncw November 28, 2018 11:43
@corona10
Copy link
Collaborator Author

@ncw
I'd like to add a unit test for this.

list(ax for x in range(10))

Can you recommend the proper place?

@codecov-io
Copy link

codecov-io commented Nov 28, 2018

Codecov Report

Merging #38 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
- Coverage   65.25%   65.24%   -0.02%     
==========================================
  Files          56       56              
  Lines       10148    10150       +2     
==========================================
  Hits         6622     6622              
- Misses       3064     3066       +2     
  Partials      462      462
Impacted Files Coverage Δ
py/sequence.go 20.43% <0%> (-0.45%) ⬇️

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 0773b68...bddb908. Read the comment docs.

@corona10
Copy link
Collaborator Author

corona10 commented Nov 28, 2018

@ncw PTAL
The coverage of unit test does not relevant with this PR

Copy link
Collaborator

@ncw ncw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you've found a good place for the test :-)

I put a few little changes inline.

The coverage of unit test does not relevant with this PR

I would have thought it would be pretty well covered.

Sequence.Iterate does not handle exceptions.
Iterate() should return error except StopIteration.
@corona10
Copy link
Collaborator Author

@ncw
Thanks for the review. I've updated the PR.
PTAL

@ncw
Copy link
Collaborator

ncw commented Nov 29, 2018

That looks great thanks :-)

I'll merge it now.

It might be worth reveiwing other calls to py.Next as that fix looks like it was left over from a refactor when it used to return a boolean rather than an error.

@ncw ncw merged commit 734fbaa into go-python:master Nov 29, 2018
@corona10 corona10 deleted the gh-32 branch November 29, 2018 12:44
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants