Allow configuring of the i_overlay Rayon transitive dep. Document more features.
#1250
Conversation
…ocument more features.
frewsxcv
force-pushed
the
frewsxcv-FBB010A0-E64C-4F4D-9DFB-90FD568B1155
branch
from
d350dbf
to
6e5b719
Compare
frewsxcv
changed the title
i_overlay dep, Rayon transitive dep, and document more features.i_overlay dep, Rayon transitive dep. Document more features.
geo/Cargo.toml
Outdated
| @@ -30,7 +31,7 @@ proj = { version = "0.27.0", optional = true } | |||
| robust = "1.1.0" | |||
| rstar = "0.12.0" | |||
| serde = { version = "1.0", optional = true, features = ["derive"] } | |||
| i_overlay = "1.7.2" | |||
| i_overlay = { version = "1.7.2", optional = true } | |||
There was a problem hiding this comment.
IMO boolean operations are such a core feature that I wouldn't oppose i_overlay itself being required. But I think the multithreading should be optional.
Also, don't you want default-features = false here, so that the i_overlay/allow_multithreading feature here is only applied when multithreading is on?
There was a problem hiding this comment.
Ah yes great catch, I meant to add default-features = false here
There was a problem hiding this comment.
IMO boolean operations are such a core feature that I wouldn't oppose i_overlay itself being required. But I think the multithreading should be optional.
I agree boolean operations are important, but the success of geo up until the operations were added to the crate indicates many people don't need that functionality and the dependency tree that comes with it. So they should have the ability to opt-out. Adding the extra cfg check here is low maintenance cost for us
There was a problem hiding this comment.
Just to be clear, we've had non optional boolean operations in geo for years now. So if anyone is building geo with no-default features, this will be a breaking change for them.
I personally have a slight preference for "make rayon optional but bool_ops mandatory" - because of the above breakage and also the additional configuration and documentation complexity outweighs the benefits of letting people fine tune, in this case, in my estimation.
But it's only a slight preference.
Question: if we break the no-default-features build, do we need to make another bump to 0.X.0?
There was a problem hiding this comment.
if anyone is building geo with no-default features, this will be a breaking change for them.
That hadn't occurred to me. I'm now also in the "slight preference for mandatory" camp.
There was a problem hiding this comment.
I still have a preference for making the bool ops stuff optional but it sounds like there's agreement about making it required so I'll just change it
geo/src/lib.rs
Outdated
| //! - `use-proj`: Enables coordinate conversion and transformation of `Point` geometries using the [`proj` crate] | ||
| //! - `use-serde`: Allows geometry types to be serialized and deserialized with [Serde] | ||
| //! - `i_overlay`: | ||
| //! - Enables the `i_overlay` crate, which provides boolean operations on geometries. |
There was a problem hiding this comment.
Can we call this feature bool_ops? I think it will be more meaningful to our users.
There was a problem hiding this comment.
Assuming we won't have a separate boolean operations implementation in the future that doesn't depend on i_overlay
There was a problem hiding this comment.
Given how long it's taken to get to this point, that seems unlikely. Not impossible, but a small risk IMO.
There was a problem hiding this comment.
In some hypothetical world where we get a second boolops implementation, we'd probably be revisiting the feature flags anyway.
There was a problem hiding this comment.
In case my original concern wasn't clear: I think a user perusing the feature flags will more quickly understand what a bool_ops feature does, vs. an i_overlay feature.
The fact that we're using i_overlay for bool_ops is an implementation detail that the user probably doesn't care about.
There was a problem hiding this comment.
Cool I'll change it to bool_ops
There was a problem hiding this comment.
Nevermind, I'm just going to remove the feature
| use-proj = ["proj"] | ||
| proj-network = ["use-proj", "proj/network"] | ||
| use-serde = ["serde", "geo-types/serde"] | ||
| multithreading = ["i_overlay/allow_multithreading"] |
There was a problem hiding this comment.
This multithreading feature also enables i_overlay - is that intended?
If so, then I think we should call it something like i_overlay_multithreading (or per my other comment, bool_ops_multithreading)
Though I could imagine a use case for a multithreading flag in geo more generally. In which case this name is good as is! But we should change the definition to:
multithreading = ["i_overlay?/allow_multithreading"]
Which enables allow_multithreading on i_overlay if we're using i_overlay, but otherwise don't enable i_overlay just because someone tried to build geo with multithreading.
There was a problem hiding this comment.
My own preference is:
multithreading = ["i_overlay?/allow_multithreading"]
There was a problem hiding this comment.
I'll change it! I didn't know about this syntax
|
Can you push up a changelog entry @frewsxcv? |
|
Yep will do that now |
frewsxcv
changed the title
i_overlay dep, Rayon transitive dep. Document more features.i_overlay Rayon transitive dep. Document more features.
CHANGES.mdif knowledge of this change could be valuable to users.