-
Notifications
You must be signed in to change notification settings - Fork 5
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
More consistent setters #124
Conversation
@Tobias-Dwyer this seems like a good PR for you to review :) |
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.
All changes look Rolex to me :D . Thanks for adding the volume setters. Can you do the same thing for setting the surface area? just scale it? (if so I could probably go in and add it)
@Tobias-Dwyer Let’s open an issue and do surface areas / perimeters / etc. in a separate PR? I think the scope of this is complete as it is. (Fix all existing setters and add just the ones I needed immediately.) |
Yep, I agree :D |
@Tobias-Dwyer Issue created: #125 |
Description
I made property setters look the same:
value
.if value > 0:
), error raising is always second (else: raise
). This is my preference because it ensures the expected path is first (easier to read) and the condition matches the error message text (if value > 0
matches the errorValue must be greater than zero.
and is thus easier to think through).I also added setters for ellipse area and ellipsoid volume. (I specifically needed ellipsoid volume but wanted ellipse area to match.)
Motivation and Context
Resolves #114. I did not know the issue already existed and had been assigned to @Tobias-Dwyer, I just wanted to add the volume setter for ellipsoids and figured I'd improve the code base at the same time.
Types of Changes
1The change breaks (or has the potential to break) existing functionality.
Checklist: