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

Use built-in ISO 8601 parsing #331

Closed
daveisfera opened this issue Oct 12, 2023 · 6 comments · Fixed by #336
Closed

Use built-in ISO 8601 parsing #331

daveisfera opened this issue Oct 12, 2023 · 6 comments · Fixed by #336

Comments

@daveisfera
Copy link
Contributor

Continuing in the vein of #38, the built-in parsing of ISO 8601 that was added in Python 3.7 should be used. This would require bumping the minimum supported Python version, but 3.8 is the oldest version that's still supported, so that should be a fine thing to do. I'd be glad to submit a PR for this, if it will be accepted

@bbayles
Copy link
Contributor

bbayles commented Oct 13, 2023

One thing to consider: if you do performance profiling, you'll find that date parsing is one of the slowest things this library does. The ciso8601 package is a C extension that speeds up this process greatly.

Right now it's easy to patch in ciso8601 for iso8601 (m3u8.parser.cast_date_time = ciso8601.parse_datetime).

I support dropping the iso8601 dependency, but it would be good to maintain the ability to patch in ciso8601.

@daveisfera
Copy link
Contributor Author

According to the benchmarks on ciso8601, it's a significant improvement over iso8601 (almost 100x), but compared to the built-in function it's a lot closer, so the need to change that should go down a lot with this change and would still be completely possible

@daveisfera
Copy link
Contributor Author

On a semi-related note, I added a PR for adding 3.12 to the list of tested versions and I noticed that ciso8601 doesn't have wheel packages for that yet, so it would need to be built from source until those are added

@daveisfera
Copy link
Contributor Author

Looks like this isn't a viable solution until Python 3.11 so I'm guessing this needs to wait

@movermeyer
Copy link

movermeyer commented Oct 16, 2023

FWIW, there is also the option of using backports.datetime_fromisoformat, which brings Python 3.11's parsing code to older versions of Python 3 (with pre-built wheels and high-performance).

@daveisfera
Copy link
Contributor Author

I did another version of my PR that uses the built-in on 3.11 and later, but I really liked the idea of using the backports (for the consistent implementation and performance improvement), so I added that to the PR as well. Thanks for the suggestion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants