Skip to content
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

Merged
merged 3 commits into from
Nov 10, 2016

Conversation

fredrikekre
Copy link
Member

Fix #19233

Think I found all the places. Also moved docstrings inline.

Copy link
Sponsor Member

@StefanKarpinski StefanKarpinski left a 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!

@fredrikekre
Copy link
Member Author

@StefanKarpinski Link to issue or PR? Seems like all entries in NEWS.md are linked to the issue, but in deprecated.jl the references are for the PR.

@StefanKarpinski
Copy link
Sponsor Member

I don't think it really matters which as long as there's an entry and a trail of breadcrumbs.

@fredrikekre
Copy link
Member Author

I don't have the power of the button, but I think this is all done?

@KristofferC
Copy link
Sponsor Member

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]).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

has -> have

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@StefanKarpinski
Copy link
Sponsor Member

There seems to be a modicum of disagreement on the original issue, so let's that discussion play out before merging this.

@martinholters
Copy link
Member

I think that discussion is resolved in favor of the change. Merge?

@KristofferC KristofferC merged commit 5fa05dc into JuliaLang:master Nov 10, 2016
@KristofferC
Copy link
Sponsor Member

Argh, I think I left the CI skip message in the squash merge. Sorry :/

@fredrikekre fredrikekre deleted the fe/numden branch November 10, 2016 14:03
@StefanKarpinski
Copy link
Sponsor Member

StefanKarpinski commented Nov 11, 2016

Not your fault: controlling the CI tools via commit messages is an incredibly dumb, bad UX design.

fcard pushed a commit to fcard/julia that referenced this pull request Feb 28, 2017
* rename num/den -> numerator/denominator

* added news entry [ci skip]

* grammar [ci skip]
Sacha0 added a commit to Sacha0/julia that referenced this pull request May 18, 2017
Sacha0 added a commit to Sacha0/julia that referenced this pull request May 18, 2017
tkelman pushed a commit that referenced this pull request Jun 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants