Repository navigation
Reach full test coverage and only support Julia 1.6 onwards - #350
PatrickHaecker wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #350 +/- ##
==========================================
+ Coverage 96.75% 99.87% +3.12%
==========================================
Files 7 7
Lines 800 786 -14
==========================================
+ Hits 774 785 +11
+ Misses 26 1 -25 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I think it would be better to split this PR into several smaller ones. The deprecated |
|
Since |
I already did the best split I could come up with in 3 commits. So the recommendation (sorry for forgetting to state it) is to review commit-by-commit. I can split the 3 commits into 3 PRs if that brings advantages I haven't seen. At least one would be very small. So without better suggestions I could reasonably go to 2. But its containing a lot of search-and-replace, so I don't expect a larger number of review findings which seem to make this useful (but again, I might have missed something).
It's common to do so. Everything before Julia 1.10 is no longer supported anyway, so it does not make any difference regarding Semver and is therefore no incompatible change. The incompatible change, if you want to call it that way, was done by dropping support for older Julia versions by Julia itself, but as that has been made transparent by Julia, I think that's all fine. I think the state with this PR is much better than before. Before the PR the package indicated to support 10 Julia versions (1.0 to 1.9) before the oldest one which was tested (1.10)! In other words, I think updating to Julia 1.6 (or 1.10) is the only reasonable thing to do. This would be aligning FixedPointNumbers.jl with packages like Revise.jl or StaticArrays.jl which also do not support older Julia versions than 1.6 (and often dropped everything which is no longer supported anyway). It's also not as if people actually using such old Julia versions would no longer have
But that was indeed the major breakthrough. Only by dropping support for older Julia versions could I reach the 100% coverage. Whether 100% coverage is a valuable goal itself is debatable and whether it's a good enough reason to drop support for older Julia versions is so, too.
Good to know. The fix for #342 can then remove the test case when being merged after #350. And the author of that PR does not need to think about whether the PR works for Julia 1.0 when being merged after #350. So this is a good example of how the current PR simplifies development. |
|
Thank you for your thoughtful comments. Regarding the additional tests, I will not review them in detail, but I would like you to commonize the implementation between |
I thought about it some more before I have seen your reply, so I might as well quickly note it down (also for my future me looking it up here): But, ok, thanks to your nice reply. I got it, so let's release it as v0.10. This will create unnecessary work for package authors whose package depends on FixedPointNumbers.jl. But as a lot of packages have not yet done the transition to v0.9 anyway, the faster we release this as v0.10, the less additional work it will create.
Done. I haven't managed to generalize all, but I think that's what you meant with "as much as possible". |
3860022 to
099a98d
Compare
Covers reductions over empty and one-element arrays, `checked_mul` for `f == 0`, the generic `AbstractFloat` conversion of `Normed`, hashing of values that equal a `Rational` (which goes through `decompose`), `%` of a foreign `Real` type, and the deprecated `floattype` fallback. The reductions, the hashing and `%` behave the same for `Fixed` and `Normed`, so common test functions check them over many raw types and fractional bits.
`Normed{UInt8,f}` requires `1 <= f <= 8`, so after the checks for
`f <= 7` only `f == 8` remains and the final `0.0f0` could never be
returned.
No CI job has tested a Julia version before 1.6 for a long time. Dropping them removes code that no tested version could run: the workarounds for `copysign`/`flipsign` with an unsigned sign argument (Julia 1.0 and 1.1), for `Base.showarg` of arrays (before 1.6) and for precompilation (1.0). Julia 1.6 also makes further workarounds unnecessary. `@exp2` and the `@generated` `inv_rawone` only forced constant folding, and plain `exp2` and division compile to identical code on Julia 1.6, 1.10 and 1.13. The `signed`/`unsigned` methods only kept Julia 1.0 from converting to an integer, and without them both still throw a `MethodError`. `Random = "1"` resolves on Julia 1.6, so the `<0.0.1` alternative is not needed. CI now tests Julia 1.6. The version becomes 0.10.0, because releases for older Julia versions end here.
099a98d to
962d561
Compare
This PR brings the codecov coverage (hopefully) to 100% while keeping the code clean and efficient.
checked_mulforf == 0, the genericAbstractFloatconversion ofNormed, hashing ofNormed(throughdecompose),%of a foreignRealtype intoNormed, and the deprecatedfloattypefallback.copysign/flipsignandBase.showargworkarounds, the precompilation version check). It also removes workarounds that Julia 1.6 no longer needs:@exp2and the@generatedinv_rawone(the generated LLVM code is identical on Julia 1.6, 1.10 and 1.13), thesigned/unsignedstubs (aMethodErroreither way) and the<0.0.1compat alternative forRandom. CI now tests Julia 1.6.Float32(::Normed{UInt8}).fis at most 8, so the final0.0f0could never be returned.The full test suite passes on Julia 1.6, 1.10 and 1.13.
Update:
This PR is now based on #365, so it will be a single commit once #365 is merged.