Skip to content

Fix all v0.7 deprecations and errors, and convert tests to testsets #108

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

Merged
merged 10 commits into from
Jun 19, 2018

Conversation

rdeits
Copy link
Contributor

@rdeits rdeits commented Jun 8, 2018

This PR fixes all deprecation warnings and errors on v0.7-alpha and changes the minimum required Julia version to v0.7-. The only errors were caused by an ambiguity resulting from the Char -> AbstractChar change, which was easy to fix. Most of the remaining deprecation warnings were because of the removed fallback from constructors to convert(). In this case, I fixed them by explicitly calling convert where appropriate. You could, instead, define (::Type{T<:Number})(x::Fixed) = convert(T, x) to restore the missing constructors. I guess it's really just a matter of deciding what the "canonical" way of doing this is.

There were also a lot of depwarns in the tests themselves because a bunch of global variables were being accidentally overwritten in for-loops. I fixed that by adding labeled testsets, which puts each test in a local scope (which should reduce the chances of one test accidentally using variables from another) and also helps document the tests themselves. I guessed at the labels, but I think I mostly got them right.

@rdeits
Copy link
Contributor Author

rdeits commented Jun 8, 2018

Alas, the github diff view really falls apart when you add a leading tab to every line in a file.... I'm going to remove the extra indentation so that the actual diff is more obvious.

@codecov-io
Copy link

codecov-io commented Jun 9, 2018

Codecov Report

Merging #108 into master will increase coverage by 10.47%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #108       +/-   ##
=========================================
+ Coverage   89.52%   100%   +10.47%     
=========================================
  Files           4      3        -1     
  Lines         191    111       -80     
=========================================
- Hits          171    111       -60     
+ Misses         20      0       -20
Impacted Files Coverage Δ
src/FixedPointNumbers.jl 100% <100%> (+15.71%) ⬆️
src/normed.jl 100% <100%> (+2.24%) ⬆️
src/fixed.jl 100% <100%> (ø) ⬆️

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 cf9ebc4...a4ee304. Read the comment docs.

REQUIRE Outdated
@@ -1,2 +1 @@
julia 0.6
Compat 0.35
julia 0.7-
Copy link
Contributor

Choose a reason for hiding this comment

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

julia 0.7-alpha.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done

@Keno
Copy link
Contributor

Keno commented Jun 10, 2018

Remove the deprecations while we're at it?

@rdeits
Copy link
Contributor Author

rdeits commented Jun 10, 2018

Ok, I've removed the deprecations. The only thing we might still want to do is change some of the convert() methods to constructors (see https://discourse.julialang.org/t/recommended-style-for-conversion-vs-constructors-in-v0-7/11561 )

@Keno
Copy link
Contributor

Keno commented Jun 13, 2018

As per that discourse thread, looks like we need to use some more constructors (and maybe define them). Other than that, this LGTM.

@rdeits
Copy link
Contributor Author

rdeits commented Jun 13, 2018

Ok, I changed all the existing convert() methods to constructors. As a bonus, removing the deprecations brings this package up to 100% test coverage!

@Keno Keno merged commit 8bfa7c2 into JuliaMath:master Jun 19, 2018
@rdeits rdeits deleted the fix-0.7 branch June 19, 2018 17:09
Sign up for free to 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