search

zeroSteiner / rule-engine

A lightweight, optionally typed expression language with a custom grammar for matching arbitrary Python objects.
https://zerosteiner.github.io/rule-engine/
BSD 3-Clause "New" or "Revised" License
455 stars 54 forks source link

Implement timedelta support #41

Closed djmattyg007 closed 2 years ago

djmattyg007 commented 2 years ago

WIP draft, I'm still updating the docs. I also need to add some unit tests for the parse_timedelta util function.

Fixes #40

djmattyg007 commented 2 years ago

@zeroSteiner I'm curious if you have any feedback. I should be able to finish updating the documentation over the next couple of days.

djmattyg007 commented 2 years ago

I wonder if I should update util.py to be _util.py?

djmattyg007 commented 2 years ago

I didn't realise this project still supported EOL versions of Python (going as far back as 3.4). Is this support still necessary? If so, I can adjust the code to be compatible :+1:

You can see where the tests are failing on old versions here: https://github.com/djmattyg007/rule-engine/runs/7114102307

zeroSteiner commented 2 years ago

This looks pretty good at first glance. I'll probably need a week or two to do a more thorough review. Really appreciate that you updated the docs and unit tests too 👍🏻

Thanks for working on this!

djmattyg007 commented 2 years ago

It's easier to explain this all in a single comment, rather than responding to multiple inline comments.

The gist of it is that I've implemented what is essentially a subset of the ISO 8601 standard for representing durations ("period" is a synonym). You can find a rundown of what this involves here:

https://en.wikipedia.org/wiki/ISO_8601#Durations

I've omitted support for "years" and "months" in this implementation, because they're ambiguous without additional information. This is also why the Python timedelta class doesn't support them.

I originally toyed with using isodate. It does have support for years and months, but it comes with some problems. This support is implemented by dynamically returning a custom Duration class that isn't fully compatible with the stdlib timedelta class. While it probably wouldn't add another dependency (it would replace dateutil instead), its parsing is stricter than dateutil's, which means it isn't a drop-in replacement.

Therefore I instead modelled the parsing code on what isodate does, but left out the parts that would result in ambiguity. I took care to replicate other spec-compliant features that isodate implements (which you can see described in the Wikipedia article). This includes supporting both . and , for decimal places.

djmattyg007 commented 2 years ago

As for resolving the issue of how these should be represented and understood by the parser, I understand why you think they should utilise prefixed strings, rather than being a raw identifier. I think I would prefer p, but I still think you should need to include the P within the string, so that the string itself is still a valid ISO 8601 duration representation. What do you think?

zeroSteiner commented 2 years ago

I think since it's a timedelta literal, I'd have an easier time remembering t as a prefix. Alternatively maybe the datatype should actually be called a period but I think that maybe more generally confusing when looking at a list of types. Timedelta is a bit more concise as to its purpose.

Agreed that it makes sense to keep P as a prefix in the string contents for consistency with the standard but since it's not strictly necessary, it'd be nice if it were optional.

That would make both of these valid literals for 1 week.

Not supporting years and months makes sense.

djmattyg007 commented 2 years ago

Sorry for the delay on finishing this up. I've been working on implementing the necessary changes, and after some testing I think my preference would be to not support the P prefix being optional.

I'll hopefully have this ready for review later today :+1:

djmattyg007 commented 2 years ago

@zeroSteiner This is ready for review :+1:

zeroSteiner commented 2 years ago

Thanks, I should be able to take a look in the next week.

zeroSteiner commented 2 years ago

Approved and merged. I ran through testing this and everything appears to be working well. I did end up renaming utils.py to _utils.py for now so it will hopefully be treated as private for the time being until I can figure out if that's where I want that code to stay.

Thanks a lot for this contribution. I'll probably post the 3.5 release this weekend.

zeroSteiner commented 2 years ago

Your changes are included in the v3.5.0 release which I just posted to PyPi. Thanks again for this great submission.