-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Add angle setter/getter to Rectangle #19646
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
Conversation
I propose to add set_ and get_ instead, but leave the storage in the public Rectangle.angle.
This is a consistency issue. We don't have attributes do fancy stuff (and thus have litte public attributes anyway). Defining set_ enables the use with standard mechanisms like rect.set(width=2, angle=30).
We don't need staling on .angle assignment. This has not been working and thus we don't need that for backward compatibility.
We may additionally deprecate .angle, but I'm undecided if that's worth it.
672fd4c to
8e9f555
Compare
My approval still stands, although you made flake8 unhappy :)
d205b68 to
6f49939
Compare
Please add tests for the new setters and getters!
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.
I don't think a test is strictly necessary for this, but of course @jklymak is right. Leaving open in case you want to add a test. If not, I'd also merge without.
Test added. It's a simple figure comparison to make sure that setting the rotation after patch creation does the same thing as setting it in the constructor, with some additional tests for get_angle().
This adds an
angleproperty, such that theRectangleis labelled stale when it is updated. All the other properties useset_andget_, but to maintain backwards compatibility I've used a property here sinceRectangle.anglealready existed.