georust / rstar

R*-tree spatial index for the Rust ecosystem
https://docs.rs/rstar
Apache License 2.0
384 stars 67 forks source link

Move PointExt::new to Point #153

Open stepancheg opened 6 months ago

stepancheg commented 6 months ago

JetBrains IDE is trying it's best to complete code, and unfortunately completes ::new for every object, because there's

impl<T> PointExt for T where T: Point {}

pub trait PointExt: Point {
    fn new() -> Self {
        Self::from_value(Zero::zero())
    }

even for types which actually don't implement Point.

We live in imperfect world, and it would be easier if rstar moved ::new to Point from PointExt. Or just hid it from public API. Perhaps #[doc(hidden)].

rmanoka commented 6 months ago

I think this is a standard way to add functionality to a trait: eg. StreamExt, FutureExt. We also have a where clause so it's not implemented for all types, only T: Point.

stepancheg commented 6 months ago

This is bad for every similar extension, but for PointExt it is especially inconvenient because of commonly used name new.

If you think this is bad idea, close the issue, I can live with it.

rmanoka commented 6 months ago

Why is it completing this function for non Point types though? That seems to be a bug iiuc.

adamreichold commented 6 months ago

I think this is a standard way to add functionality to a trait: eg. StreamExt, FutureExt. We also have a where clause so it's not implemented for all types, only T: Point.

It is standard to extend traits from foreign crates. But in this case Point and PointExt are both defined in the same module, so indeed we could just move all methods defined by PointExt into Point and drop the trait entirely.

I think this would primarily change that PointExt and the methods it defines are currently not part of the public API and I don't know if we want to commit to them.

Why is it completing this function for non Point types though? That seems to be a bug iiuc.

That said, I also think this is a bug since not only is this bound to T: Point, it is not even part of our public API.