-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make_zero(!) bugfixes and improved tests #1961
Conversation
Thanks for taking a look! I noticed a few other issues that should be fixed too. However, #1852 is almost there (I've been in a cycle of thorough tests => noticing issues => fixing/improving design accordingly => repeat), so if you don't mind I'll finish the big push on that, and then we can decide whether it's worth fixing and merging this one first (say, if reviewing #1852 is too big of a task to deal with right now), or if we should close this one and focus on #1852 |
Can you rebase this? I think I would like to get this merged first before dealing with the much larger change in #1852 |
Sure! Just give me a minute to incorporate some additional fixes and improvements I've made in the meantime |
2c673a3
to
327a2f9
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1961 +/- ##
==========================================
+ Coverage 67.50% 70.64% +3.14%
==========================================
Files 31 44 +13
Lines 12668 16122 +3454
==========================================
+ Hits 8552 11390 +2838
- Misses 4116 4732 +616 ☔ View full report in Codecov by Sentry. |
327a2f9
to
e4df3be
Compare
Aiming for full coverage of both new and old implementations of make_zero(!)
e4df3be
to
48a6e6f
Compare
56ada23
to
636b025
Compare
Since #1852 isn't quite ready, in the meantime, here's a PR fixing the bugs I found in make_zero(!) and adding a fairly thorough test suite.
The last test currently fails due to #1935. I didn't want to mark it as broken because I'd rather see #1936 merged (or #1935 fixed in some other way), but let me know if I should change that.