Add else clause and raise TypeError if incorrect unit type #11832
Conversation
|
p.s. Need to fix code style failure too. |
astropy/units/format/utils.py
Outdated
| @@ -109,6 +109,8 @@ def decompose_to_known_units(unit, func): | |||
| return decompose_to_known_units(unit._represents, func) | |||
| raise | |||
| return unit | |||
| else: | |||
| raise TypeError(f'Incorrect type {type(unit)}') | |||
There was a problem hiding this comment.
The code looks good, but I think the error message itself could be more helpful: since "incorrect type" is just repeating the name of the error. How about "unit argument must be a 'NamedUnit' or 'CompositeUnit', not {type(unit)}."
This is a bit more like for float:
In [4]: float(None)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
<ipython-input-4-b14d5e084cce> in <module>
----> 1 float(None)
TypeError: float() argument must be a string or a number, not 'NoneType'
astropy/units/tests/test_format.py
Outdated
| def test_fits_to_string_function_error(): | ||
| """Test if will raise a TypeError if incorrect unit type.""" | ||
|
|
||
| with pytest.raises(TypeError, match='Incorrect type'): |
There was a problem hiding this comment.
Need to adjust the match if we go with my suggestion above
astropy/units/tests/test_format.py
Outdated
| @@ -379,6 +379,12 @@ def test_flexible_float(): | |||
| assert u.min._represents.to_string('latex') == r'$\mathrm{60\,s}$' | |||
|
|
|||
|
|
|||
| def test_fits_to_string_function_error(): | |||
| """Test if will raise a TypeError if incorrect unit type.""" | |||
There was a problem hiding this comment.
This maybe is a good place to note why we are testing this, perhaps
"""Test function raises TypeError on bad input.
This instead of returning None, see gh-11825.
"""
|
I added "no-changelog-entry-needed" since I think it was far from easy to reach the missing code path, and in the end it led to an error (even if an unexpected one). |
|
My local branch Should I make the changes first before I do the rebase/squashed thing? |
|
@jerobado - I'd rebase & squash, and then force-push so we can look at it again. |
Add else clause and raise TypeError if incorrect unit type This will add an 'else' clause that will catch the UnboundLocalError and raise a TypeError if unit type is incorrect. Add TypeError test for Fits.to_string() function Keep two lines to the next function to conform with PEP-8 Co-authored-by: Ole Streicher <github@liska.ath.cx> Make raising of TypeError more specific Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com> Apply corrections on previous commits
jerobado
force-pushed
the
fix-issue-11825
branch
from
0b2ac92
to
64ca723
Compare
|
Great, merging! Thanks, @jerobado! |
Description
As discussed over at #11825, this will add an
elseclause that will catch theUnboundLocalErrorand raise aTypeErrorif unit type is incorrect.Just let me know if I missed something.
Fixes #11825