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

Fix comparison from wrong formats with datetime objects #2697

Merged
merged 6 commits into from
Mar 20, 2023

Conversation

stricaud
Copy link
Contributor

Make sure datetime objects are used in their timestamp format to avoid the exception "TypeError: can't compare offset-naive and offset-aware datetime" with Python 3.11

…id the exception "TypeError: can't compare offset-naive and offset-aware datetimes" with Python 3.11
@stricaud stricaud requested a review from a team as a code owner February 26, 2023 22:04
@codecov
Copy link

codecov bot commented Feb 26, 2023

Codecov Report

Patch coverage: 100.000% and project coverage change: +0.054 🎉

Comparison is base (5ee36fd) 88.607% compared to head (064f63b) 88.661%.

Additional details and impacted files
@@              Coverage Diff              @@
##              main     #2697       +/-   ##
=============================================
+ Coverage   88.607%   88.661%   +0.054%     
=============================================
  Files           87        87               
  Lines         6820      6844       +24     
  Branches      1171      1178        +7     
=============================================
+ Hits          6043      6068       +25     
  Misses         535       535               
+ Partials       242       241        -1     
Impacted Files Coverage Δ
sanic/app.py 90.068% <100.000%> (+0.315%) ⬆️
sanic/response/convenience.py 97.435% <100.000%> (+1.039%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@stricaud
Copy link
Contributor Author

stricaud commented Mar 3, 2023

Any update on this?

@sjsadowski
Copy link
Contributor

Hi there, looks good. Can you please add a test to prevent a regression?

@stricaud
Copy link
Contributor Author

stricaud commented Mar 3, 2023

This is my first time contributing to Sanic, could you please show me how to add test?

@Tronic
Copy link
Member

Tronic commented Mar 3, 2023

This is my first time contributing to Sanic, could you please show me how to add test?

Sure. Sanic uses pytest for testing (you can find guides for that online) and you can run the tests on your own machine by

# Replace py311 with your Python version (here 3.11)
tox -e py311 -- tests/test_response.py

Or alternatively directly with pytest, without tox. If you leave out the double hyphen and test filename, it will run all tests which will take some time. Before changing anything, see that the tests pass OK.

Since your patch is in sanic/request/, the appropriate test file is probably that mentioned in the example above. It might already have a test function that tests the validate_file function. You might be able to modify an existing test function, or simply add your own function named test_validate_file_datetime, where you call the function in a way that without your patch would raise an exception.

When doing bugfixes and tests for them, it is a good practice to then try running your test without your patch, to see that it actually fails, and then apply the patch and see that it passes.

Please ask if you have any further questions, we'll be happy to help getting started with that!

@Tronic
Copy link
Member

Tronic commented Mar 15, 2023

@stricaud Hey, how are you doing? Let me know if you need help with the test.

@stricaud
Copy link
Contributor Author

I am trying to write the test, however it has been many days and I don't see how to reproduce this problem.
I do not have much time. I will see if I can live with a non patched version and see the problem coming again to be able to write the test.

@ahopkins
Copy link
Member

@stricaud I think I see what you are after. Do you mind if I make some pushes to your branch?

@stricaud
Copy link
Contributor Author

I don't mind! Very welcome

@Tronic
Copy link
Member

Tronic commented Mar 18, 2023

I'd like to make sure that the fix proposed is the right one: what causes tz-aware and unaware timestamps being mixed here, can we make both either tz-aware or not aware? If not, is there a chance that one of them would be in local tz, and produce wrong timestamp value?

@ahopkins
Copy link
Member

If not, is there a chance that one of them would be in local tz, and produce wrong timestamp value?

Yup. I am not sure how best to avoid this without going too deep and over-engineering this. My vote is as the implementation I pushed: handle when they are both aware and both naive. If there is a mismatch, add the missing tz as UTC and throw up a warning.

@ahopkins ahopkins merged commit 0099540 into sanic-org:main Mar 20, 2023
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.

4 participants