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

Take &mut self instead of self in builders #22

Closed
frozolotl opened this issue Sep 21, 2019 · 9 comments
Closed

Take &mut self instead of self in builders #22

frozolotl opened this issue Sep 21, 2019 · 9 comments

Comments

@frozolotl
Copy link

See https://rust-lang-nursery.github.io/api-guidelines/type-safety.html#builders-enable-construction-of-complex-values-c-builder

@IvanUkhov
Copy link
Member

Could you please be more specific? Which methods are you referring to?

@frozolotl
Copy link
Author

I'm referring to most methods in svg::node::element.
It would allow writing for example:

let mut document = Document::new();
if cond {
    document.add();
}

@IvanUkhov
Copy link
Member

I think it’s a good suggestion. I’ve tagged it as “feedback needed” to see if there are other opinions so that we don’t miss anything. If you have time, please feel free to open a pull request.

@MichaelMauderer
Copy link

I just ran into the same issue. So I'm just here to pile on.

I have a wrapper struct that is meant to hold a Document and manipulate it.
Since add takes the Document by value, this means that a cumbersome dance with Options or mem::replace is needed to make this work.

Example:

struct SVGRenderer {
    document: Document,
}

impl SVGRenderer {
    pub fn new() -> Self {
        SVGRenderer {
            document: Document::new(),
        }
    }

    fn add_to_doc<T: Node>(&mut self, obj: T) {
        self.document = mem::replace(&mut self.document, Document::new()).add(obj);
    }
}

impl Renderer for SVGRenderer {
    fn draw_point(&mut self, p: &Point) {
        let circle = Circle::new()
            .set("cx", p.pos.x)
            .set("cy", p.pos.y)
            .set("r", p.radius)
            .set("stroke", "black")
            .set("stroke-width", 1);
        // Can't do self.document.add(circle)
        // Neither self.document = self.document.add(circle)
        // Needs to be something like this
        self.document = mem::replace(&mut self.document, Document::new()).add(obj);
    }
} 

@IvanUkhov
Copy link
Member

Let us continue this discussion in #23. I need your input. Thanks!

@IvanUkhov
Copy link
Member

@MichaelMauderer, by the way, in your example, would it not work with append?

@IvanUkhov
Copy link
Member

@frozolotl, can’t append help you too?

@MichaelMauderer
Copy link

Actually, it does! I had completly missed that function. Thanks for pointing it out.

@IvanUkhov
Copy link
Member

I park it for now, as add and set are accompanied by append and assign, respectively.

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

Successfully merging a pull request may close this issue.

3 participants