whoosh-community / whoosh

Whoosh is a fast, featureful full-text indexing and searching library implemented in pure Python.

Other

240 stars 36 forks source link

Correct FP rounding errors #539

Open stevennic opened 5 years ago

stevennic commented 5 years ago

Following up on #519, correcting a few more rounding bugs I found.

codecov[bot] commented 5 years ago

Codecov Report

Merging #539 into master will increase coverage by 0.01%. The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #539      +/-   ##
==========================================
+ Coverage   82.99%   83.01%   +0.01%     
==========================================
  Files         132      132              
  Lines       29621    29621              
==========================================
+ Hits        24585    24590       +5     
+ Misses       5036     5031       -5

Impacted Files	Coverage Δ
src/whoosh/matching/binary.py	`71.88% <0%> (ø)`	:arrow_up:
src/whoosh/collectors.py	`92.64% <0%> (-0.22%)`	:arrow_down:
tests/test_collector.py	`99.41% <0%> (+0.58%)`	:arrow_up:
src/whoosh/index.py	`76.01% <0%> (+1.55%)`	:arrow_up:

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 9c020a7...d755fb1. Read the comment docs.

stevennic commented 5 years ago

I can't explain the codecov/patch problem. Is it a deal breaker for merging?

nijel commented 5 years ago

0% coverage for patch, should mean that you changes are not covered by tests at all. It would be great if you could add tests covering those cases.

stevennic commented 5 years ago

I always do, but in this case I just changed 3 logical operators in existing code, so I don't see how coverage could change at all. I read some issues in codecov forums echoing similar strange behavior with 0% codecov/patch but I can't tell if it's related.

nijel commented 5 years ago

The coverage has not changed (what can be seen on codecov/project check). The problem is that the code you've changed is not covered by tests at all.

PS: I'm not saying this should block accepting this, I'm just explaining why Codecov complains.

stevennic commented 5 years ago

I see, so it was uncovered all along?

nijel commented 5 years ago

Exactly. You've touched code paths which are not tested at all. It's not that hard as there is about 30% of that file not covered by tests, see https://codecov.io/gh/whoosh-community/whoosh/src/master/src/whoosh/matching/binary.py

stevennic commented 5 years ago

Ah I see! Good opportunity to add unit tests then. I'll try to get to that. Thanks for the clarification.

stevennic commented 5 years ago

I'm unable to find a unit test that covers this code path at this time. Given that this patch fixes a found bug with minimal changes, I propose it be merged anyway. We can work on increasing code coverage separately.