Skip to content

builtin: Implement builtin sum - #21

Merged
corona10 merged 1 commit into
go-python:masterfrom
corona10:builtin_sum
Sep 4, 2018
Merged

builtin: Implement builtin sum#21
corona10 merged 1 commit into
go-python:masterfrom
corona10:builtin_sum

Conversation

@corona10

@corona10 corona10 commented Aug 31, 2018

Copy link
Copy Markdown
Collaborator

Implement built-in sum.
@corona10
corona10 requested review from ncw and sbinet August 31, 2018 15:06
@codecov-io

codecov-io commented Aug 31, 2018 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #21 into master will increase coverage by 0.05%.
The diff coverage is 82.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #21      +/-   ##
==========================================
+ Coverage    64.5%   64.55%   +0.05%     
==========================================
  Files          55       55              
  Lines        9740     9768      +28     
==========================================
+ Hits         6283     6306      +23     
- Misses       3010     3013       +3     
- Partials      447      449       +2
Impacted Files Coverage Δ
builtin/builtin.go 75.2% <82.14%> (+0.57%) ⬆️

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 ed3c651...ba56f58. Read the comment docs.
@corona10

corona10 commented Aug 31, 2018

Copy link
Copy Markdown
Collaborator Author

@ncw PTAL
sbinet
sbinet reviewed Aug 31, 2018
View reviewed changes
Comment thread builtin/tests/builtin.py
@corona10

corona10 commented Sep 1, 2018

Copy link
Copy Markdown
Collaborator Author

@sbinet I've updated PTAL
@corona10

corona10 commented Sep 1, 2018

Copy link
Copy Markdown
Collaborator Author

@ncw Can you take a look?
ncw
ncw requested changes Sep 3, 2018
View reviewed changes

@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.

I think this OK apart from the tests not passing under py3test. If you rebase to master then you'll get the testing stuff in travis and you can see what I mean.
Comment thread builtin/tests/builtin.py Outdated
try:
sum(['h', 'i'])
except TypeError as e:
if e.args[0] != "unsupported operand type(s) for +: 'int' and 'string'":

@ncw ncw Sep 3, 2018

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.

This fails under python3 because the error message is actually

TypeError: unsupported operand type(s) for +: 'int' and 'str'

I'll work on getting py3test into the CI
@corona10

corona10 commented Sep 3, 2018

Copy link
Copy Markdown
Collaborator Author

@ncw
Your CL is awesome!
I 've rebased the master and fix a code which is related to failing.
PTAL
sbinet
sbinet approved these changes Sep 4, 2018
View reviewed changes

@sbinet sbinet left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would probably leave the s/"string"/"str"/ change in its own commit, but ok.

LGTM.
@ncw

ncw commented Sep 4, 2018

Copy link
Copy Markdown
Collaborator

Look great now - thanks :-)
@corona10
corona10 merged commit 09f14d0 into go-python:master Sep 4, 2018
@corona10
corona10 deleted the builtin_sum branch September 4, 2018 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants

@corona10 @codecov-io @ncw @sbinet