from and as RangeBounds

[Eh2406]

needs better docs and tests, but what do you think?

[mpizenberg]

Looks good! Maybe instead of as_range_bound, we could name it bounding_range or bounding_interval or bounding_bounds or bounding_box since generally in 2D or 3D that's called the bounding box of the surface/volume.

Now we have to see how using this looks in all the tests and examples.

[mpizenberg]

I'm having a go at refactoring of examples with this.

[Eh2406]

I am trying to use as_range_bounds with a BTreeMap in OfflineDependencyProvider, it may need some changes to be useful.
A reduction of the problem is https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=697bd6544b479b4f2b248b99dfc630bf

[mpizenberg]

I'm probably only going to try using the From impl for now.

[mpizenberg]

I'm having a go at refactoring of examples with this.

So actually I'm having a hard time using this in ways that feel more natural than what we currently have. Even when trying different ways to write the from functions generically. So I'll just wait for what you come up with

[Eh2406]

I was able to use as_range_bounds for the OfflineDependencyProvider, but it was not a perf win.

How is the from_range_bounds for examples? Better then range_from_bounds copied into each project?

[mpizenberg]

How is the from_range_bounds for examples? Better then range_from_bounds copied into each project?

I didn't try the advanced dependency provider repo. Instead I tried using this in the examples/ directory but I wasn't happy at all with the results. I either didn't manage to make things compile or it was too bulky. I don't have the code anymore, I was so unhappy that I deleted it ^^. Let me know with what you get if you find an enjoyable way to use it in those examples. (I started with doc_interface.rs)

[Eh2406]

Yes, we where missing some Froms to make this work, we can now:

    let t: Range<SemanticVersion> = Range::strictly_lower_than((2, 0, 0));
    assert_eq!(t, Range::from_range_bounds(&(..(2, 0, 0))));
    
    let t: Range<SemanticVersion> = Range::higher_than((2, 0, 0));
    assert_eq!(t, Range::from_range_bounds(&((2, 0, 0)..)));

    let t: Range<SemanticVersion> = Range::between((1, 0, 0), (2, 0, 0));
    assert_eq!(t, Range::from_range_bounds(&((1, 0, 0)..(2, 0, 0))));

The &( ) is annoying but I am not getting the lifetimes to work without it.
So how bad is that?

[Eh2406]

I was able to use as_range_bounds for the OfflineDependencyProvider, but it was not a perf win.

This testing should probably be re-done after rust-lang/rust#86031

[mpizenberg]

Could this introduce stealthy clones in future code?

[Eh2406]

Good thought. In this case I think it is ok.
For one thing .from() and .into() should probably always be thought of as "potentially as costly as a clone".
For another this is not generic, and we know the cost to clone a SemanticVersion is small.
What do you think?

[mpizenberg]

This testing should probably be re-done after rust-lang/rust#86031

I'm not sure to follow what this is about.

[Eh2406]

rust-lang/rust#86031 improves Std's BTree* to do less work in constructing a iterator. The tests of use as_range_bounds for the OfflineDependencyProvider hinged on whether it was faster to:

• use .iter() on a BTreeMap witch iterates over all of the collection but does no comparisons to construct.
• use .range(r.as_range_bounds()) on a BTreeMap witch iterates over only the needed parts, but does O(ln(N)) comparisons to construct.

1/2 the number of comparisons may favor the .range version, or it may not matter. But it is something to test again, when we have a chance.

[Eh2406]

I did the renaming, made clippy happy, and got the life times working!

    let t: Range<SemanticVersion> = Range::strictly_lower_than((2, 0, 0));
    assert_eq!(t, Range::from_range_bounds(..(2, 0, 0)));
    
    let t: Range<SemanticVersion> = Range::higher_than((2, 0, 0));
    assert_eq!(t, Range::from_range_bounds((2, 0, 0)..));

    let t: Range<SemanticVersion> = Range::between((1, 0, 0), (2, 0, 0));
    assert_eq!(t, Range::from_range_bounds((1, 0, 0)..(2, 0, 0)));

now works!

[mpizenberg]

Sorry about delay. I've had very busy weeks. I'll have some time to look at this again the weekend next week but most likely not before.

[Eh2406]

I made the change for strictly_lower_than, higher_than, and between in the examples. It is about the same ergonomics from_range_bounds vs bespoke methods, although the consistency is nice.
any, none, and exact can also be moved over but the ergonomics are so much worse that I did not move the examples.

The ergonomic winds that justify this PR do not appear in examples:

    let t: Range<SemanticVersion> = Range::strictly_lower_than((2, 0, 0)).union(&Range::exact((2, 0, 0)));
    assert_eq!(t, Range::from_range_bounds(..=(2, 0, 0)));

    let t: Range<SemanticVersion> = Range::between((1, 0, 0), (2, 0, 0)).union(&Range::exact((2, 0, 0)));
    assert_eq!(t, Range::from_range_bounds((1, 0, 0)..=(2, 0, 0)));

The other major argument for this is that it can be used in generic contexts.

[mpizenberg]

Looks good to me! I made small docs changes.