Closed Eh2406 closed 2 years ago
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.
I'm having a go at refactoring of examples with this.
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
I'm probably only going to try using the From
impl for now.
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
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?
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
)
Yes, we where missing some From
s 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?
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 https://github.com/rust-lang/rust/pull/86031
This testing should probably be re-done after rust-lang/rust#86031
I'm not sure to follow what this is about.
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:
.iter()
on a BTreeMap
witch iterates over all of the collection but does no comparisons to construct..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.
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!
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.
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.
needs better docs and tests, but what do you think?