Skip to content

builtin,vm: add implementation for builtin hex function - #123

Merged
ncw merged 4 commits into
go-python:masterfrom
sbinet-gonum:impl-builtin-hex
Nov 12, 2019
Merged

builtin,vm: add implementation for builtin hex function#123
ncw merged 4 commits into
go-python:masterfrom
sbinet-gonum:impl-builtin-hex

Conversation

@sbinet

Copy link
Copy Markdown
Member

No description provided.

@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #123 into master will decrease coverage by 0.16%.
The diff coverage is 3.44%.

Impacted file tree graph

@@ Coverage Diff @@## master #123 +/- ##
==========================================
- Coverage 72.74% 72.57% -0.17% 
==========================================
Files 60 60 Lines 11970 11999 +29 ==========================================
+ Hits 8707 8708 +1 - Misses 2729 2757 +28 
Partials 534 534
Impacted FilesCoverage Δ
builtin/builtin.go74.07% <3.44%> (-3.46%)⬇️

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 051f189...3835a01. Read the comment docs.

@sbinet
sbinet requested review from corona10 and ncw and removed request for ncwNovember 5, 2019 14:33
@sbinet

Copy link
Copy Markdown
MemberAuthor

does anybody understand why e.g.builtin_isinstance and builtin_hex are marked as "not covered" while e.g.builtin_sorted is?

For the life of me, I just don't understand.

@ncw

ncw commented Nov 5, 2019

Copy link
Copy Markdown
Collaborator

Codecov seems to do mad things sometimes! Did you force push your branch? I think that makes it worse sometimes.

I wonder if there is a retry button for codecov?

@sbinet

Copy link
Copy Markdown
MemberAuthor

no, I didn't force push. (at least, not for this PR :P)
and actually running go test -cover also shows the same behaviour wrt builtin_hex and builtin_sorted.

@ncw

ncw commented Nov 5, 2019

Copy link
Copy Markdown
Collaborator

Very strange. I can't see anything wrong with your code. Might be worth trying with different go versions to see if it is a go bug.

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

lgtm

@sbinet

Copy link
Copy Markdown
MemberAuthor

ok. figured it out.

the tests (for builtin_hex and builtin_isinstance) were in the wrong location (and actually badly written) :)

@sbinet

Copy link
Copy Markdown
MemberAuthor

@ncw PTAL

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

Look great now :-)

I'll merge it

@ncw
ncw merged commit efe8d6a into go-python:masterNov 12, 2019
@sbinet
sbinet deleted the impl-builtin-hex branch August 4, 2021 08:47
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@sbinet@codecov-io@ncw@corona10