-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Rename num/den to numerator/denominator #19246
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.
Needs an entry in NEWS.md. Looks good otherwise!
@StefanKarpinski Link to issue or PR? Seems like all entries in |
I don't think it really matters which as long as there's an entry and a trail of breadcrumbs. |
I don't have the power of the button, but I think this is all done? |
LGTM |
@@ -66,6 +66,8 @@ Deprecated or removed | |||
|
|||
* `is` has been deprecated in favor of `===` (which used to be an alias for `is`) ([#17758]). | |||
|
|||
* `num` and `den` has been deprecated in favor of `numerator` and `denominator` respectively ([#19233]). |
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.
has -> have
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.
fixed
There seems to be a modicum of disagreement on the original issue, so let's that discussion play out before merging this. |
I think that discussion is resolved in favor of the change. Merge? |
Argh, I think I left the CI skip message in the squash merge. Sorry :/ |
Not your fault: controlling the CI tools via commit messages is an incredibly dumb, bad UX design. |
* rename num/den -> numerator/denominator * added news entry [ci skip] * grammar [ci skip]
Fix #19233
Think I found all the places. Also moved docstrings inline.