Useful error message for TimeFromEpoch scale conversion error. #2046
Conversation
|
|
||
| with pytest.raises(ScaleValueError) as exc: | ||
| t = Time(Time.now().cxcsec, format='cxcsec', scale='ut1') | ||
| assert str(exc) == ScaleValueError |
There was a problem hiding this comment.
This assert statement needs to be in the outer block, not in the with pytest.raises() as exc: block. The reason is that the t = Time() statement raises the exception and the assert statement never gets executed. If it did, then you would see a failure (see next comment).
There was a problem hiding this comment.
str(exc) will be the text of the error message, so the line can be
assert str(exc).startswith("Cannot convert from 'cxcsec' epoch "
"scale 'tt' to specified scale 'ut1', got error:")
In this case you don't need to explicitly check the entire rest of the error message.
|
@taldcroft I should be apologising for not taking PEP8 into consideration even after reading about it. |
|
@kaichogami No problem about PEP8: It has a lot of pedantic little rules that nobody can be expected to learn exactly all at once, and it's not hard to forget or miss something. The reason it's easy to over time is that most Python code in the public (in particular anything written in the last 5 year or so) tries to adhere to it. So at a certain point, once you've stared at enough Python code, it just "looks PEP8" or not without thinking about it. |
|
I understand. I suppose its like learning a new language. I will get used to it, hopefully. |
|
@kaichogami Click the "Details" link: https://travis-ci.org/astropy/astropy/builds/18288472 There are several different "builds". Look for the column called "Env": The |
|
@embray Yes the tests earlier were failing. I should have double checked them. On closer inspection I felt that there was no need for the "assert statement". I changed it accordingly. |
| @@ -1249,7 +1249,7 @@ class TimeFromEpoch(TimeFormat): | |||
| def __init__(self, val1, val2, scale, precision, | |||
| in_subfmt, out_subfmt, from_jd=False): | |||
| self.scale = scale | |||
| # Initialize the reference epoch which is a single time defined in subclasses | |||
| # Initialize the reference epoch which is a single time defined in subclasses | |||
There was a problem hiding this comment.
These whitespace changes should be removed from the patch.
|
Sorry for making it review again and again, I will make sure I don't make same mistakes in the next PR. I hope this settles all the PEP8 issues. If there is still something I missed please let me know. |
| @@ -310,6 +310,9 @@ Bug Fixes | |||
|
|
|||
| - ``astropy.time`` | |||
|
|
|||
| - Fix a problem where scale conversion problem in TimeFromEpoch | |||
| was not showing a useful error. | |||
There was a problem hiding this comment.
Just one more thing that we should have mentioned earlier. The pull request issue number should be included, so it should be was not showing a useful error. [#2046].
|
I have been trying, but for some reason, its not passing the Travis Cl build. I dont really know why is this happening. I just added [#2046] and its not passing the test. I would appreciate help now. |
|
@astrofrog or @eteq - This is giving a Travis fail on Numpy 1.5, but I think it's unrelated to this PR. Can you try re-running the test? |
|
@taldcroft - restarted, but there's a few builds ahead in the queue so it might be a bit. |
|
And its working. |
Useful error message for TimeFromEpoch scale conversion error.
|
@kaichogami - merged! |
|
Thank you! My first successful pull request ever! :) |
|
Congratulations and thanks for your contribution! |
Pull request for the updated error message in case of failure in scale conversion in TimeFromEpoch. First pointed in issue #1829 (now closed).
@taldcroft please review these changes and tell me if any changes are required!
Thank you!