Skip to content

Deprecate emu unit - #5906

Merged
pllim merged 2 commits into
astropy:masterfrom
pllim:deprecate-emu
Mar 28, 2017
Merged

pllim merged 2 commits into
astropy:masterfrom
pllim:deprecate-emu

Conversation

@pllim

@pllim pllim commented Mar 23, 2017

Copy link
Copy Markdown
Member

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

mhvk commented Mar 26, 2017

Copy link
Copy Markdown
Contributor

@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

pllim commented Mar 27, 2017

Copy link
Copy Markdown
Member Author

@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

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

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

mhvk commented Mar 27, 2017

Copy link
Copy Markdown
Contributor

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

@MSeifert04

Copy link
Copy Markdown
Contributor

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

@pllim

pllim commented Mar 27, 2017

Copy link
Copy Markdown
Member Author

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

@bsipocz

bsipocz commented Mar 27, 2017 •

Copy link
Copy Markdown
Member

@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

pllim commented Mar 27, 2017

Copy link
Copy Markdown
Member Author

@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

pllim commented Mar 28, 2017

Copy link
Copy Markdown
Member Author

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.

4 participants