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

Deprecate emu unit #5906

Merged
merged 2 commits into from Mar 28, 2017
Merged

Deprecate emu unit #5906

merged 2 commits into from Mar 28, 2017

Conversation

pllim
Copy link
Member

@pllim pllim commented Mar 23, 2017

Fix #4918 , where user was confused by the "emu" unit due to ambiguous definition of that term.

Supersede #5787 .

A subset of #5793. And this follows one of the suggestions there by @mhvk . Perhaps @eteq can utilize this new module for his work on #5661.

@pllim pllim added this to the v2.0.0 milestone Mar 23, 2017
@pllim pllim requested a review from mhvk March 23, 2017 20:12
@pllim pllim force-pushed the deprecate-emu branch 2 times, most recently from 916c252 to 44505db Compare March 24, 2017 17:02
@mhvk
Copy link
Contributor

mhvk commented Mar 26, 2017

@pllim - I like this approach! I wonder, though, whether it wouldn't make sense to follow imperial a bit more closely and just define the units and use add_enabled_units in enable. In the comments you note possible name clashes. Are you thinking of something specific? The prefixed earth/jupiter mass? I think this might well "just work" even with add_enabled_units... (but have not checked).

Also, obviously needs some mention in the documentation...

@pllim
Copy link
Member Author

pllim commented Mar 27, 2017

@mhvk

Re: "add" vs "set". No particular reason. It was just cut-and-paste. And apparently I chose the wrong module. Fixed that in the second commit.

Re: Documentation. It is auto-generated. The following will appear at the end of the API documentation. Given that the unit is deprecated, I decided against giving it extra publicity, as we do not want to encourage people to use it (but it is there if they really, really want to).

screenshot

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

This now all looks good!

@mhvk
Copy link
Contributor

mhvk commented Mar 27, 2017

@pllim - OK, all looks good. Feel free to merge once the tests have passed.

@MSeifert04
Copy link
Contributor

hm, deprecated.enable() sounds like a very good 2017-4-1 feature. 😄

@pllim
Copy link
Member Author

pllim commented Mar 27, 2017

We can always wait till Apr 1 to merge, it is not that far away. 😂

@bsipocz
Copy link
Member

bsipocz commented Mar 27, 2017

@pllim @mhvk - I agree that this doesn't need much publicity, thus I wonder whether we even need the table of the deprecated units, or the description above the table should be enough?

@pllim
Copy link
Member Author

pllim commented Mar 27, 2017

@bsipocz , then there is no easy way to find out what unit is deprecated. Also, this provides a consistent API presentation. So, I am against removing the table but I will if overruled.

@pllim
Copy link
Member Author

pllim commented Mar 28, 2017

Since this is approved and tests passed, merging.

@pllim pllim merged commit e0b1bc5 into astropy:master Mar 28, 2017
@pllim pllim deleted the deprecate-emu branch March 28, 2017 16:42
@eteq eteq mentioned this pull request Jun 16, 2017
aarchiba added a commit to aarchiba/astropy that referenced this pull request Sep 6, 2019
aarchiba added a commit to aarchiba/astropy that referenced this pull request Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants