Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise error for invalid reference date for encoding time units #5288

Merged
merged 4 commits into from
May 13, 2021

Conversation

gcaria
Copy link
Contributor

@gcaria gcaria commented May 10, 2021

Although the error raised by this commit does not include the whole units string, I believe it is actually more useful and specific since it focuses on the part (reference date) that's actually causing the problem.
Also, the reference date is the only information available in coding.times._ensure_padded_year, so this is the simplest way of raising an error.

I had a look in tests/test_coding_times.py and could not find a test for this related error raising (since I'd put a test for this commit around there)

matches = re.match(r"(.+) since (.+)", units)
if not matches:
raise ValueError(f"invalid time units: {units}")

Have I missed it?

EDIT: I've tried to substitute the error raise on line 129 with a pass and all the tests passed anyway.

@mathause
Copy link
Collaborator

Looks good, thanks. As usual a test would be superb! You can add a new test in

def test_encode_cf_datetime_overflow(shape):

It can be as easy as

with pytest.raises(ValueError, match="invalid reference date"):
    xr.coding.times.encode_cf_datetime([1, 2, 3], units='days since NO_YEAR')

@pep8speaks
Copy link

pep8speaks commented May 12, 2021

Hello @gcaria! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-05-13 17:43:10 UTC

@gcaria gcaria force-pushed the error_message_time_units branch from 6d3eaa6 to a46936f Compare May 12, 2021 18:32
@gcaria
Copy link
Contributor Author

gcaria commented May 12, 2021

Done :) Since it was very little effort, I've also added a test for the other related error raising.

@gcaria gcaria force-pushed the error_message_time_units branch from a46936f to eab4fa8 Compare May 13, 2021 07:29
@max-sixty max-sixty added the plan to merge Final call for comments label May 13, 2021
@mathause
Copy link
Collaborator

Sorry I wasn't entirely clear - I meant "the tests belong in this general vicinity" - could you put them in their own function? You can also give yourself credit by adding something to whats-new (up to you).

dcherian added 2 commits May 13, 2021 11:40
…e_units

* upstream/master:
  combine keep_attrs and combine_attrs in apply_ufunc (pydata#5041)
  Explained what a deprecation cycle is (pydata#5289)
  Code cleanup (pydata#5234)
  FacetGrid docstrings (pydata#5293)
  Add whats new for dataset interpolation with non-numerics (pydata#5297)
  Allow dataset interpolation with different datatypes (pydata#5008)
  Flexible indexes: add Index base class and xindexes properties (pydata#5102)
@dcherian dcherian mentioned this pull request May 13, 2021
9 tasks
@dcherian
Copy link
Contributor

Thanks @gcaria!

@dcherian dcherian merged commit 0a29ef8 into pydata:master May 13, 2021
@gcaria
Copy link
Contributor Author

gcaria commented May 13, 2021

Hi @dcherian, thanks for the merge, however I think there was one last edit left for this PR, i.e. put the tests in their own function. How should I go about this?

@max-sixty
Copy link
Collaborator

@gcaria no problem at all to submit another PR — the more the merrier

@gcaria gcaria deleted the error_message_time_units branch May 14, 2021 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better error message for setting encoding["units"]
5 participants