-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Add new constructors for Circle and Sphere
#11526
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
Conversation
IQuick143
left a comment
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.
| 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.
| 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
newconstructor forCircleandSphere.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
newalso increases consistency.Ellipsehasnewalready, and so does the mesh version ofCircle.Solution
Add a
newconstructor forCircleandSphere.