Skip to content

py: Fix errors are suppressed in generator comprehensions#38

Merged
ncw merged 1 commit intogo-python:masterfrom
corona10:gh-32
Nov 29, 2018
Merged

py: Fix errors are suppressed in generator comprehensions#38
ncw merged 1 commit intogo-python:masterfrom
corona10:gh-32

Conversation

@corona10
Copy link
Collaborator

@corona10 corona10 commented Nov 28, 2018

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

corona10 commented Nov 28, 2018

@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 •
edited
Loading

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 •
edited
Loading

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

ncw
ncw requested changes Nov 28, 2018
View reviewed changes
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.

@corona10
py: Fix errors are suppressed in generator comprehensions
bddb908 
Sequence.Iterate does not handle exceptions.
Iterate() should return error except StopIteration.
@corona10
Copy link
Collaborator Author

corona10 commented Nov 28, 2018

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ncw ncw ncw approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@corona10 @codecov-io @ncw