Query datetime parser

[discopatrick]

This pull request adds new feature #2506 (Extend the query date parser with (optional) times).

I have some concerns/questions about my code:

1. Period.parse() works, but has become quite a mess: https://github.com/discopatrick/beets/blob/query-datetime-parser/beets/dbcore/query.py#L565-L577

   This is due to the fact that we now define multiple date formats per precision (i.e. separating the date and time with a 'T' or a space character) - e.g.: https://github.com/discopatrick/beets/blob/query-datetime-parser/beets/dbcore/query.py#L541-L543

   Thus, we have a nested loop - the outer loop iterates over precisions, and the nested loop iterates over the formats for the current precision. I use break statements to jump back out of the loops once a datetime is successfully parsed, however, this requires an extra flag variable found, and due to the order in which statements are executed, I have to manually decrease the ordinal variable - very ugly.

   Can anyone advise on how to make this cleaner?

2. I've added tests for the different datetime formats (i.e. 'T' vs space separator): https://github.com/discopatrick/beets/blob/query-datetime-parser/test/test_datequery.py#L182-L199 - would others agree this is the correct place for these tests?

3. Do we need additional unit tests that test Period.parse() directly, or will it suffice to continue testing at a higher level by creating an instance of DateQuery as we do now?

4. (edited to add:) I guess it should be me who updates the docs for this feature too.

[mention-bot]

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

[Kraymer]

Thus, we have a nested loop - the outer loop iterates over precisions, and the nested loop iterates over the formats for the current precision. I use break statements to jump back out of the loops once a datetime is successfully parsed, however, this requires an extra flag variable found, and due to the order in which statements are executed, I have to manually decrease the ordinal variable - very ugly.

Can anyone advise on how to make this cleaner?

What about using an inner function that enable you to use a return statement in your nested for loops ?
Something like :

@classmethod
def parse(cls, string):

    def find_date_and_format(string):
       for ordinal, date_format in enumerate(cls.date_formats):
           for format_option in date_format:
               try:
                   date = datetime.strptime(string, format_option)
                   return date, ordinal
               except ValueError:
                   # Parsing failed.
                   pass
       return (None, None)

    if not string:
        return None
    date, ordinal = find_date_and_format(string)
    if date is None:
        raise InvalidQueryArgumentTypeError(string,
                                            'a valid datetime string')
    precision = cls.precisions[ordinal]
    return cls(date, precision)

[sampsyo]

Nice! This is looking great! I'm excited to use this myself.

1. It looks quite a bit tidier now thanks to @Kraymer's suggestion. Any outstanding concerns about complexity?
2. Yep, that's the right place for the tests.
3. I would say no. It wouldn't be a bad thing to test the new code at that abstraction level, but doing it at the query level is fine too—and both seems unnecessary.
4. If you don't mind adding the docs, that would be awesome! Thanks again! ✨

[discopatrick]

@sampsyo - docs are now completed.

[Kraymer]

what about the "but before 23:00" part?

[discopatrick]

I assume you're asking why there isn't a .. in the query, followed by the right hand part of the range?

This is because by specifying the hour of 22, results from anywhere within that hour are returned. The right hand part of the range is implied.

This is the same way that date queries already work.

[Kraymer]

ok but the query returns items added between 23:00 and 0:00

[Kraymer]

Sorry, confusion from my part with queries with the .. at the end.
Cool you added the case in the test though !

[discopatrick]

No problem! It might be worth having a non-range test for all precisions?

[sampsyo]

Thank you for all your work on this, @discopatrick! This is a nice, clean extension and I like the positive effect it has on the simplicity of the date query parsing code. Awesome work.

Since you've done a great job contributing a couple of PRs, I'm inviting you to the @beetbox org. That should let you commit directly if that ever seems helpful. Opening pull requests is still fine, of course, if you ever want a code review.

[discopatrick]

@sampsyo - I'm honoured!

Really pleased to be contributing to beets. It looks like it's about to become my primary tool for keeping my music collection organised, so I hope to be coming back to write more new features in the future.

In the meantime, will 1.4.5 be released any time soon? If so, I'll wait and update via pip. If not, I'll install beets from the master branch.

[sampsyo]

Fantastic! I'm glad you're interested in the project.

Yep, I do plan on releasing 1.4.5 sooner rather than later—I have something of a backlog of pull requests to review, but once that's done, I'll spin out a new release.