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

Support ordered duration strings for timedelta #366

Merged
merged 5 commits into from
Nov 14, 2024

Conversation

ddelange
Copy link
Contributor

@ddelange ddelange commented Nov 12, 2024

Duration strings like 7m7s are widely used for configs, especially anything kubernetes related (anything go-related really, because of the time.ParseDuration builtin).

This PR adds a simple yet robust regex parser for such strings.

For timedelta specifically, this also incorporates #297

@ddelange ddelange force-pushed the duration-timedelta branch 2 times, most recently from a754a45 to e5345ff Compare November 12, 2024 18:09
@sloria
Copy link
Owner

sloria commented Nov 12, 2024

Thanks for the PR! This is interesting. Do you have a source from which you derived the regex?

I'm open to this feature, but it'd be good to have a spec or a source that we can refer to if we need to update it.

@ddelange
Copy link
Contributor Author

ddelange commented Nov 13, 2024

Hi @sloria 👋

Thanks for the quick reply!

This PR loosely adopts the GEP-2257 spec (which has been widely adopted by the infra/devops community) to Python timedelta units:

  • The go ParseDuration implementation allows h|m|s|ms|[uµ]s|ns.
  • GEP-2257 spec allows h|m|s|ms
  • ISO 8601 time interval looks like P3Y6M4DT12H30M5S (P for period, T for time, and then Y|M|D|H|M|S, note duplicate M).
  • Python timedelta supports subsets of both, plus weeks, arriving at w|d|h|m|s|ms|[uµ]s for this PR.

The PR supports only the units which are supported by Python timedelta. So no month / year / nanoseconds. Avoids month/minute ambiguity as well (also, how to convert month to timedelta: 1 month == 30.5 days?)

I wrote the pattern myself and added the comments for maintainability. Discrepancies with the GEP-2257 duration strings:

  • units explained above
  • this pattern allows for optional whitespace around the units (see the tests)
  • this pattern is now compiled to be case-insensitive, because why not
    • ISO 8601 durations are strictly uppercase
    • GEP-2257 duration strings are strictly lowecase
  • GEP-2257 duration strings don't have to be ordered (10s30m1h), current pattern expects ordering (1h30m10s)
  • GEP-2257 duration strings can contain duplicate units (100ms200ms300ms)

@sloria
Copy link
Owner

sloria commented Nov 13, 2024

Thanks, that's very helpful info 👍 . Above the regex, can you add a link to the spec and add a comment block with the discrepancies you listed?

@@ -1,6 +1,6 @@
# Changelog

## 11.1.0 (2025-11-11)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

@ddelange
Copy link
Contributor Author

@sloria

  • docs added
  • had another look at the spec and added support for 0s 0h etc
  • had another look at the marshmallow implementation and added support for -42s etc
  • expanded tests for these cases, added some expected failures

@sloria
Copy link
Owner

sloria commented Nov 14, 2024

Thank you! Looks great!

Is there a good reason to make the regex case-insensitive? It could be more future-proof to keep it case-sensitive. Uppercase characters are probably a typo by the user anyway, right?

@ddelange
Copy link
Contributor Author

Sure thing, done 💥

@sloria
Copy link
Owner

sloria commented Nov 14, 2024

Thank you! This looks good to go

@sloria sloria enabled auto-merge (squash) November 14, 2024 21:07
@sloria sloria merged commit 031cb9b into sloria:main Nov 14, 2024
8 checks passed
@sloria
Copy link
Owner

sloria commented Nov 14, 2024

Released in 11.2.0

@ddelange
Copy link
Contributor Author

awesome, thanks for the collab and for quickly landing this!
landed

@ddelange ddelange changed the title Support ordered case-insensitive duration strings for timedelta Support ordered duration strings for timedelta Nov 15, 2024
@tcleonard
Copy link

tcleonard commented Nov 15, 2024

This was a breaking change...
The following code used to work with 11.1.0 and now fail with the error TypeError: expected string or bytes-like object, got 'float'

from environs import Env
env = Env()

toto = env.timedelta("TOTO", default=30.0)

Now it takes a string and it can be only an int in it:

toto = env.timedelta("TOTO", default=30)   # does not works
toto = env.timedelta("TOTO", default=30.0)   # does not works
toto = env.timedelta("TOTO", default="30")   # works
toto = env.timedelta("TOTO", default="30.")   # does not works

To be fair, passing a float before was working but in a strange way cause if you were providing 30.5 it was returning a 30 seconds time delta (ignoring the 0.5)

@ddelange
Copy link
Contributor Author

interesting, marshmallow expects a int for timedelta, and this PR switched to subclassing TimeDelta. maybe there was some lenience that was silently swallowing the float

@tcleonard
Copy link

I've submitted a PR to propose a fix #368

@sloria
Copy link
Owner

sloria commented Nov 20, 2024

Fix released in 11.2.1. Thanks for the report and the fix!

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