Refactored the implementation of RangeSet.

[ThouCheese]

Refactored the implementation of RangeSet. The original implementation did not enforce privacy of the fields of Range, which meant that the implementation of Range could not be easily swapped out. I also added tests for all functions of RangeSet and Range. I also added several extra functions to Range that make the implementation of RangeSet a bit more legible (thing like writing range1.touches(range2) instead of range1.start() <= range2.end() && range1.end() >= range2.start()). I also favoured operator overloading over method calls, so instead of range1.minus(range2) we can write range1 - range2. I hope this PR is welcome! Let me know what you think.

[Johannesd3]

There are some clippy warnings:

warning: This sequence of operators looks suspiciously like a bug.
   --> audio/src/range_set.rs:223:49
    |
223 |             if to_sub.start() <= cur.start() && cur.start() < to_sub.end() {
    |                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: I think you meant: `cur.end() < to_sub.end()`
    |
    = note: `#[warn(clippy::suspicious_operation_groupings)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_operation_groupings

warning: This sequence of operators looks suspiciously like a bug.
   --> audio/src/range_set.rs:223:49
    |
223 |             if to_sub.start() <= cur.start() && cur.start() < to_sub.end() {
    |                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: I think you meant: `cur.end() < to_sub.end()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_operation_groupings

warning: use Vec::sort_by_key here instead
  --> audio/src/range_set.rs:45:9
   |
45 |         ranges.sort_unstable_by(|r1, r2| r1.start().cmp(&r2.start()));
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `ranges.sort_unstable_by_key(|r1| r1.start())`
   |
   = note: `#[warn(clippy::unnecessary_sort_by)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_sort_by

error: suspicious use of binary operator in `Sub` impl
   --> audio/src/range_set.rs:276:14
    |
276 |         self += rhs;
    |              ^^
    |
    = note: `#[deny(clippy::suspicious_arithmetic_impl)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_arithmetic_impl

error: aborting due to previous error; 3 warnings emitted

[ThouCheese]

Ah I had seen only the first one, which is a false positive. The last two ones however are not! I fixed this and added #![allow(clippy::suspicious_operation_groupings)] to range_set.rs.

[Johannesd3]

Tbh I am not convinced of all your changes, especially not the big ones.

That operator overloading would be a better choice is just one point of view. One could also argue that "adding" and "subtracting" range set are rather union and intersection of sets, so the symbols don't really fit. Since the old api worked fine, your suggestion is not objectively better, and it's not even public, I would tend to keeping it as it was.

Another change that I don't understand are the changes to Range. Using start and an unsigned length had the advantage that every possible value would be valid. Also I don't quite understand your arguments against making it fields public.

There are some improvements, but the big changes distract from those. It would be much easier to review if the diff would be smaller.

The documentations and tests are really welcome, you certainly noticed the lack of it in this repository.

My suggestion: Cut down some of your changes. If you still think there are some bigger changes necessary, let's discuss it here and add those back in separate commits.

[ThouCheese]

Hi, sorry for the delayed response, but thanks for your straightforward review. I will try to reduce the diff by removing the following: operator overloading and the range with start and end. Then from there on you can say if you think the diff is manageable or whether I should cut it down even further.

[ThouCheese]

I have reduced the diff somewhat, but a large part remains due to the privacy of start and end. I can remove that as well, but I believe that there is some sense of value in privacy when working with structs, namely:

1. The classical OOP argument of encapsulation. In range set, we can now not care about whether length, or start or end are a field or a function, but instead we are forced to access these 3 properties in a consistent way.
2. We can easily make modifications to the Range struct without breaking any further code in range_set.rs or fetch/*.

If you're not convinced I'll take it out and revert to the original implementation with 2 field accesses and 1 function of course, let me know!

[roderickvd]

@ThouCheese can you rebase so we can let the checks run and merge on success?

[ThouCheese]

Hi, thanks for the ping! I had totally forgotten about this change. I will take a look at the @Johannesd3 left, resolve them and rebase.

[ThouCheese]

I have resolved all comments but one, which I did not fully understand, and rebased.

[JasonLG1979]

The OCD in me sees this PR open every time I go to the librespot repo and wonders if it will ever be merged? It seems to have stalled out 6 months ago?

[roderickvd]

I hope @Johannesd3 returns.

[roderickvd]

I am sorry that this went into disrepair and the work that you put into this.

Cleaning up, I will close for now, but will be absolutely happy to go forward with this if you or others would update it.