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

@corona10corona10 commented Aug 31, 2018

Copy link
Copy Markdown
Collaborator

Implement built-in sum.

@corona10
corona10 requested review from ncw and sbinetAugust 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 FilesCoverage Δ
builtin/builtin.go75.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
CollaboratorAuthor

@ncw PTAL

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

corona10 commented Sep 1, 2018

Copy link
Copy Markdown
CollaboratorAuthor

@sbinet I've updated PTAL

@corona10

corona10 commented Sep 1, 2018

Copy link
Copy Markdown
CollaboratorAuthor

@ncw Can you take a look?

ncw
ncw requested changes Sep 3, 2018
View reviewed changes

@ncwncw 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 threadbuiltin/tests/builtin.py Outdated
try:
sum(['h', 'i'])
except TypeError as e:
if e.args[0] != "unsupported operand type(s) for +: 'int' and 'string'":

@ncwncwSep 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
CollaboratorAuthor

@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

@sbinetsbinet 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 approved these changes Sep 4, 2018
View reviewed changes
@ncw

ncw commented Sep 4, 2018

Copy link
Copy Markdown
Collaborator

Look great now - thanks :-)

@corona10
corona10 merged commit 09f14d0 into go-python:masterSep 4, 2018
@corona10
corona10 deleted the builtin_sum branch September 4, 2018 14:48
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ncwncwncw approved these changes
@sbinetsbinetsbinet 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.

4 participants

@corona10@codecov-io@ncw@sbinet