search

invenia / Intervals.jl

Non-iterable ranges
MIT License
36 stars 18 forks source link

Drop all v1 deprecations for v2 #214

Open rofinn opened 1 year ago

rofinn commented 1 year ago

Removes all deprecations placed in src/deprecations and fixes/deletes any relevant tests. Commits are split up by types of deprecations being dropped.

@omus I can't seem to set you as a reviewer, but I'm guessing you're the primary person who should review these changes?

codecov[bot] commented 1 year ago

Codecov Report

Merging #214 (eb2afc1) into v2-DEV (ba938f6) will increase coverage by 9.54%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           v2-DEV     #214      +/-   ##
==========================================
+ Coverage   84.31%   93.85%   +9.54%     
==========================================
  Files          12       10       -2     
  Lines         848      700     -148     
==========================================
- Hits          715      657      -58     
+ Misses        133       43      -90

see 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

omus commented 1 year ago

I've allocated some bandwidth to review this tomorrow

omus commented 1 year ago
  1. Do we want to maintain v1 deserialization in v2 (it'll be a lot of work/bloat for all the proposed breaking changes)?

I'd make an issue against the v2 milestone and deal with this before we make the official v2.0.0 release. I don't think we need to keep deserialization compatibility with Intervals 1.2 as that version is quite old but being able to load Intervals 1.9.0 would be nice to have in Intervals 2.0.0. I would 100% punt this work until the internal redesign is done though as otherwise it'll be annoying to update while the internal design is being iterated on.

  1. Do we want to include tests of Base behaviour (ie: that union(intervalsets) != union(vectors))?

Yes, as long as we're testing other Base set functions in comparisons.jl we should should also include this test. We could remove the vector of intervals tests in which case we would also remove the test you mentioned. Maybe what we should so is define methods like Base.union(::AbstractVector{Interval} = throw(MethodError) to stop end users from mistakenly using these methods? In that case I'd be fine with removing the vector of intervals tests.