-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add new
constructors for Circle
and Sphere
#11526
Add new
constructors for Circle
and Sphere
#11526
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a good straightforward change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very small doc changes to match the other doc comments, otherwise looks good!
Edit: forgot to expand the files so I didn't see that some other doc comments in the file weren't ending phrases with a dot, IMO we should always add one for consistency but there doesn't seems to be fixed guideline for that at the moment, so feel free to ignore if you want.
@@ -88,6 +88,12 @@ pub struct Circle { | |||
impl Primitive2d for Circle {} | |||
|
|||
impl Circle { | |||
/// Create a new [`Circle`] from a `radius` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Create a new [`Circle`] from a `radius` | |
/// Create a new [`Circle`] from a `radius`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most constructors (Plane2d::new
, Segment2d::new
, Polyline2d::new
, Triangle2d::new
, Rectangle::new
...) in these files currently don't end the doc comment with a period unless the comment spans multiple lines. The doc comments for Circle
and its radius also don't end with a period. I'd like to change them all to end with a period and be more consistent though.
@@ -92,6 +92,12 @@ pub struct Sphere { | |||
impl Primitive3d for Sphere {} | |||
|
|||
impl Sphere { | |||
/// Create a new [`Sphere`] from a `radius` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Create a new [`Sphere`] from a `radius` | |
/// Create a new [`Sphere`] from a `radius`. |
# Objective Make APIs more consistent and ergonomic by adding a `new` constructor for `Circle` and `Sphere`. This could be seen as a redundant "trivial constructor", but in practise, it seems valuable to me. I have lots of cases where formatting becomes ugly because of the lack of a constructor, like this: ```rust Circle { radius: self.radius(), } .contains_local_point(centered_pt) ``` With `new`, it'd be formatted much nicer: ```rust Circle::new(self.radius()).contains_local_point(centered_pt) ``` Of course, this is just one example, but my circle/sphere definitions very frequently span three or more lines when they could fit on one. Adding `new` also increases consistency. `Ellipse` has `new` already, and so does the mesh version of `Circle`. ## Solution Add a `new` constructor for `Circle` and `Sphere`.
Objective
Make APIs more consistent and ergonomic by adding a
new
constructor forCircle
andSphere
.This could be seen as a redundant "trivial constructor", but in practise, it seems valuable to me. I have lots of cases where formatting becomes ugly because of the lack of a constructor, like this:
With
new
, it'd be formatted much nicer:Of course, this is just one example, but my circle/sphere definitions very frequently span three or more lines when they could fit on one.
Adding
new
also increases consistency.Ellipse
hasnew
already, and so does the mesh version ofCircle
.Solution
Add a
new
constructor forCircle
andSphere
.