Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move PointExt::new to Point #153

Open
stepancheg opened this issue Jan 12, 2024 · 4 comments
Open

Move PointExt::new to Point #153

stepancheg opened this issue Jan 12, 2024 · 4 comments

Comments

@stepancheg
Copy link

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
Copy link
Contributor

rmanoka commented Jan 12, 2024

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
Copy link
Author

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
Copy link
Contributor

rmanoka commented Jan 12, 2024

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

@adamreichold
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants