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 compare datetime with diffrent timezones #159

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mobinghoveoud
Copy link

@mobinghoveoud mobinghoveoud commented Oct 26, 2024

fix #158

Description of Changes

This PR enhances the comparison operations in the datetime class, similar to the Python datetime module, to resolve issues with time comparisons involving different timezones.

Testing

To ensure the accuracy of the changes, I added several tests to the project. Since the main comparison logic is handled in the _cmp method, primary tests were written for this function. Additionally, I added tests for methods like __eq__, __gt__, etc., by mocking the _cmp method as needed.

If there are any issues or areas for improvement, please feel free to let me know.
Thanks.

@mobinghoveoud
Copy link
Author

I added another commit to fix a test failure on AppVeyor.

On Windows, you just need to install tzdata to use zoneinfo and no extra imports are needed.

@slashmili
Copy link
Owner

Thanks @mobinghoveoud!

I'm wondering if it's easier to convert the data to python's datetime and let python module compare the dates!

@mobinghoveoud
Copy link
Author

mobinghoveoud commented Oct 28, 2024

I'm wondering if it's easier to convert the data to python's datetime and let python module compare the dates!

Thanks for reply @slashmili .
I considered this approach but there may be some performance concerns with too many comparisons. Commonly, we’re comparing two jdatetime instances, so each comparison requires two conversions. Letting python's datetime module handle the comparisons would certainly make implementation easier.

If performance is a priority, the current method works well but if easier implementation and maintenance are more important, the conversion approach could be a feasible alternative. I agree with both approaches.

@hramezani
Copy link
Collaborator

Thanks @slashmili for asking my opinion here.

The change LGTM 👍

I agree with @mobinghoveoud that the current implementation has better performance, but the data-time approach needs less maintenance effort in the future. It's up to you, @slashmili, to decide.

@slashmili
Copy link
Owner

Sorry for @mobinghoveoud for getting to you late. I'd prefer to not keep the logic and rely on python datetime. I'd say speed of comparison has lower priority compare to maintainability of this project.

@mobinghoveoud
Copy link
Author

mobinghoveoud commented Nov 15, 2024

Thanks, I'll try this approach.

@mobinghoveoud
Copy link
Author

To compare the datetimes, first converted to python datetime objects.

  • For equality __eq__, the locales are checked first. If they are different, it returns False.
  • The __ne__ method was removed since it already relies on __eq__.
  • Tests have been updated.

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.

Datetime comparison bug with different timezones
3 participants