Skip to content

Conversation

@sbinet
Copy link
Member

No description provided.

@codecov-io
Copy link

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.

@sbinetsbinet requested review from corona10 and ncw and removed request for ncwNovember 5, 2019 14:33
@sbinet
Copy link
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
Copy link
Collaborator

ncw commented Nov 5, 2019

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

ncw commented Nov 5, 2019

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.

Copy link
Collaborator

@corona10corona10 left a comment

Choose a reason for hiding this comment

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

lgtm

@sbinet
Copy link
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
MemberAuthor

@ncw PTAL

Copy link
Collaborator

@ncwncw left a comment

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

@ncwncw merged commit efe8d6a into go-python:masterNov 12, 2019
@sbinetsbinet 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