Skip to content

[EXP] Add deprecation warning for emu CGS unit - #5787

Closed
pllim wants to merge 1 commit into
astropy:masterfrom
pllim:deprecate-emu
Closed

[EXP] Add deprecation warning for emu CGS unit#5787
pllim wants to merge 1 commit into
astropy:masterfrom
pllim:deprecate-emu

Conversation

@pllim

@pllim pllim commented Feb 9, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Here is an idea for deprecating emu, as discussed in #4918. With this change, the warning will be emitted on import:

>>> import astropy  # no warning
>>> from astropy import units as u  # has warning
WARNING: AstropyDeprecationWarning: emu is deprecated...

This might not be a good idea, since we turn all deprecation warnings into exceptions in our tests. Hence, that is why I skipped CI in the commit, as I am pretty sure things will fail spectacularly.

I looked at units/core.py and could not find an easy way to only emit warning when u.emu is called. I tried adding a new keyword called deprecated_aliases in def_unit() but that turned complicated really fast, as I had to propagate that new keyword to __init__ and __call__ in several different places. Not sure if it is worth the while to add so many LOCs just to deprecate a unit.

Anyway, if nothing else, this PR serves as a reminder and a place for discussions until a better one supersedes it. c/c @mhvk , @bsipocz

TODO (if we decide to proceed):

  • Add change log.
  • Add test.

@pllim pllim added this to the v2.0.0 milestone Feb 9, 2017
@pllim pllim changed the title Added deprecation warning for emu CGS unit Add deprecation warning for emu CGS unit Feb 9, 2017
@pllim pllim changed the title Add deprecation warning for emu CGS unit [EXP] Add deprecation warning for emu CGS unit Feb 9, 2017
@bsipocz

bsipocz commented Feb 9, 2017

Copy link
Copy Markdown
Member

I'm sorry, but I'm very much against emitting the warning whenever units is imported. I suspect that hardly anyone is using this unit (I have to admit that I have never heard of it before this issue came up), while the warning will bite everyone. Getting rid of it without a deprecation seems less painful than the global warning.

I had an idea at the time of the discussion in #4918, but never finished it up, will try and dig up the memories and figure out what it was and why it never worked.

@pllim

pllim commented Feb 10, 2017

Copy link
Copy Markdown
Member Author

The underlying implication is that it won't be easy to deprecate prefix of, say, solarmass, as well. core.py is written in such a generic way that it is non-trivial to just deprecate a certain aspect of a certain unit.

Perhaps, we can approach this another way. Instead of issuing deprecation warning, we can remove it right away as you said, but also keep the old version in a new module like deprecated_units.py. That way, if people want backward compatibility, they simply need to import the deprecated version of the unit from that module... But that also can cause confusion.

@mhvk mhvk mentioned this pull request Feb 10, 2017
Closed
@mhvk

mhvk commented Feb 10, 2017

Copy link
Copy Markdown
Contributor

I think this is indeed not a good idea, and I'll close this now as I think we should not have the discussion here, but rather in a place where we will still find it --> #5793 (which I turned the project note into and where I also comment on a possible solution).

@mhvk mhvk closed this Feb 10, 2017
@pllim
pllim deleted the deprecate-emu branch February 10, 2017 19:55
@pllim pllim mentioned this pull request Mar 23, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Experimental Last-before-release units

Projects

None yet

Milestone

v2.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@pllim @bsipocz @mhvk