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

Date value field validation #2517

Merged
merged 9 commits into from
Apr 19, 2017

Conversation

discopatrick
Copy link
Member

This pull request is for issue #2513 (Date-valued field queries allow non-date values).

Period.parse() now raises a InvalidQueryArgumentTypeError when the string does not parse to a date - although I do wonder whether this is the correct error to raise, as I'm not comparing types here. I wonder whether a ValueError, or perhaps creating a new InvalidQueryArgumentValueError is more appropriate?

I have changed the way Period.parse() works - it used to first count the number of hyphens in the date to determine the precision (day, month, or year) and then try to parse the string against the appropriate date format. This did work fine. However, in anticipation of new feature #2506 (Extend the query date parser with (optional) times), where we will have '-', ':', and possibly ' ' or 'T' characters to count, I have opted for a different approach. I now test the string against each of the formats supported, from most precise to least precise, until a match is found (or not).

Tests have been updated to assert that the new errors are raised.

The output of a bad date query now reads quite nicely in the output:

$ beet list added:2017-01..2017-aa
invalid query: 'added:2017-01..2017-aa': '2017-aa' is not a valid datetime string

Finally, I have fixed a couple of older tests which were attempting to test mtime queries with an odd looking value of 0.0 and updated them to correct values.

tox passes all tests.

@mention-bot
Copy link

@discopatrick, thanks for your PR! By analyzing the history of the files in this pull request, we identified @sampsyo, @pkess and @diego-plan9 to be potential reviewers.

@sampsyo
Copy link
Member

sampsyo commented Apr 15, 2017

Yay! Thanks for getting this started!

Period.parse() now raises a InvalidQueryArgumentTypeError when the string does not parse to a date - although I do wonder whether this is the correct error to raise, as I'm not comparing types here. I wonder whether a ValueError, or perhaps creating a new InvalidQueryArgumentValueError is more appropriate?

Sure. In fact, I think we only use this exception as more akin to ValueError, not TypeError. So perhaps we should just rename the exception.

I have changed the way Period.parse() works -

Would you mind putting this refactoring into a separate pull request? Having one logical change at a time makes things a lot easier to review.

@discopatrick
Copy link
Member Author

Sure @sampsyo, I'm splitting into two pull requests now.

I will leave the renaming of the exception to yet another pull request, to keep things tidy.

@sampsyo
Copy link
Member

sampsyo commented Apr 19, 2017

Awesome; thanks! ✨ I merged this one with a changelog entry.

@sampsyo sampsyo merged commit 18c5128 into beetbox:master Apr 19, 2017
sampsyo added a commit that referenced this pull request Apr 19, 2017
sampsyo added a commit that referenced this pull request Apr 19, 2017
This was referenced Apr 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants