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

Replaces datetime.fromisoformat with the more lenient dateutil parser #8507

Merged
merged 2 commits into from
Sep 19, 2023
Merged

Replaces datetime.fromisoformat with the more lenient dateutil parser #8507

merged 2 commits into from
Sep 19, 2023

Conversation

stumpylog
Copy link
Contributor

Note: Before submitting this pull request, please review our contributing
guidelines
.

Description

Fixes #8499. The isoparse from dateutil is more correct about what it supports, handling the 'Z' timezone as well, which earlier versions of Python didn't support.

I'm not 100% sure where the user is providing the eta as a string to, since apply_async notes it to be a datetime. If someone can point me to that and a simple test case, I'll add some additional date strings there to catch this in the future.

@codecov
Copy link

codecov bot commented Sep 16, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (a4fa400) 87.44% compared to head (053998b) 87.45%.
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8507      +/-   ##
==========================================
+ Coverage   87.44%   87.45%   +0.01%     
==========================================
  Files         148      148              
  Lines       18491    18499       +8     
  Branches     3156     3158       +2     
==========================================
+ Hits        16169    16179      +10     
+ Misses       2033     2032       -1     
+ Partials      289      288       -1     
Flag Coverage Δ
unittests 87.42% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
celery/app/base.py 96.75% <100.00%> (+<0.01%) ⬆️
celery/result.py 96.70% <100.00%> (+<0.01%) ⬆️
celery/utils/iso8601.py 87.50% <100.00%> (ø)
celery/utils/time.py 96.93% <100.00%> (+0.01%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@auvipy auvipy requested review from auvipy and a team September 17, 2023 06:41
@Nusnus
Copy link
Member

Nusnus commented Sep 17, 2023

This change seems to make sense but we need proper testing added.

@Nusnus
Copy link
Member

Nusnus commented Sep 17, 2023

I'm not 100% sure where the user is providing the eta as a string to, since apply_async notes it to be a datetime. If someone can point me to that and a simple test case, I'll add some additional date strings there to catch this in the future.

Are you still blocked?

@stumpylog
Copy link
Contributor Author

Yes, I'm not familiar enough to know where I would add some datetime strings to verify more iso formats can be parsed. Everything I found was using an actual datetime already, so it's format was fine before and after.

@Nusnus
Copy link
Member

Nusnus commented Sep 17, 2023

Yes, I'm not familiar enough to know where I would add some datetime strings to verify more iso formats can be parsed. Everything I found was using an actual datetime already, so it's format was fine before and after.

TBH I am also not familiar that much with this part of the code, but check this out, maybe this module is what you're looking for:

def test_maybe_iso8601_datetime():

Linking to a specific (relevant) function that possibly could help you ease into testing the new PR, but check the rest of the module for additional context @stumpylog

@stumpylog
Copy link
Contributor Author

I've added additional coverage of maybe_iso8061, including Z and +xx:yy string parsing.

@Nusnus
Copy link
Member

Nusnus commented Sep 18, 2023

I've added additional coverage of maybe_iso8061, including Z and +xx:yy string parsing.

Great!
I'll review it later today.

Thank you!

Copy link
Member

@Nusnus Nusnus left a comment

Choose a reason for hiding this comment

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

LGTM

Nice unit tests 💯

@auvipy auvipy added this to the 5.3.x milestone Sep 19, 2023
@auvipy auvipy merged commit bbe8775 into celery:main Sep 19, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ETA field on task is not correctly parsed in celery>=5.3.0 and python<3.11
3 participants