Repository navigation
Deprecate unused units.format.Unscaled format - #14417
Conversation
mhvk
added
units
API change
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Just a few in-line comments to help review.
| if isinstance(unit, core.CompositeUnit): | ||
| parts = [] | ||
|
|
||
| if cls._show_scale and unit.scale != 1: |
There was a problem hiding this comment.
Removing this means I can refactor to_string elsewhere. But here really it just serves to make everything else independent of the deprecated Unscaled class.
There was a problem hiding this comment.
My understanding of a separate deprecatation PR would be to have one just with the @deprecated flag in (and some followups like removed tests). Is there a specific reason to have other code changes here?
There was a problem hiding this comment.
I think it is OK, as here it really helps to ensure that once the deprecation period is over, all one has to do is to remove the class (one could argue about the test, but since I just added it, I think it is fine to remove it already!).
| _show_scale = False | ||
| @classmethod | ||
| def to_string(cls, unit): | ||
| if unit.scale != 1: |
There was a problem hiding this comment.
This just ensures there is no scale to show.
| "format_spec, string", | ||
| [ | ||
| ("generic", "dB(1 / m)"), | ||
| ("unscaled", "dB(1 / m)"), |
There was a problem hiding this comment.
This was added only in #14413, so no big loss to remove it. Otherwise, "unscaled" was not used at all!
olebole
left a comment
olebole
left a comment
There was a problem hiding this comment.
I think this is quite useful (I didn't fully understand the use of that formatting); so 👍
pllim
left a comment
pllim
left a comment
There was a problem hiding this comment.
I am approving by proxy but will let someone else do the merge. Thanks, everyone!
Description
This pull request is to deprecate the unused "unscaled" format in units. I split this off from #14416 since on looking at that PR again, I realized it is just becoming very long and hard to see the logic. Plus, one should not really have API changes in refactoring PRs.