Skip to content

py: Fix errors are suppressed in generator comprehensions - #38

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

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

Conversation

@corona10

Copy link
Copy Markdown
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
Copy Markdown
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

codecov-io commented Nov 28, 2018

Copy link
Copy Markdown

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

corona10 commented Nov 28, 2018

Copy link
Copy Markdown
Collaborator Author

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

@ncw ncw left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread py/sequence.go
Comment thread vm/tests/generators.py
Comment thread vm/tests/generators.py Outdated
Sequence.Iterate does not handle exceptions.
Iterate() should return error except StopIteration.
@corona10

Copy link
Copy Markdown
Collaborator Author

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

@ncw

ncw commented Nov 29, 2018

Copy link
Copy Markdown
Collaborator

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

Labels

None yet

3 participants