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

Work on DateTime for Python #3663

Merged
merged 31 commits into from
Jan 23, 2024
Merged

Work on DateTime for Python #3663

merged 31 commits into from
Jan 23, 2024

Conversation

MangelMaxime
Copy link
Member

Related to #3654

@MangelMaxime
Copy link
Member Author

MangelMaxime commented Dec 14, 2023

This is strange the CI is failing on one of the test while I can run the tests locally. I tested with the same version of Python 3.10 on the dev container which is also on ubuntu.

Edit: One difference I can see is that I am running Python 3.10 an the CI is running CPython 3.10

@dbrattli
Copy link
Collaborator

CPython is the same as Python. Almost all Pythons are CPython, except PyPy, Cython, Jython, ... Usually (every) time you talk about Python, you talk about CPython.

But can there be any locale format differences between your local computer and CI?

@MangelMaxime
Copy link
Member Author

Thanks for the explanations.

About the locales, I am not sure. Because, in theory 2014-09-10T13:50:34.0000000 should be handled by datetime.fromisoformat(string).astimezone() as this is an ISO format date.

If it was related to locales, I would expect the error to happens later like when we try to compare the date value or something like that.

@dbrattli
Copy link
Collaborator

dbrattli commented Dec 16, 2023

It is related to Python 3.10. It parses for more recent versions of Python. I made a fix.

Copy link
Collaborator

@dbrattli dbrattli left a comment

Choose a reason for hiding this comment

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

Looks good. Should we merge?

@MangelMaxime
Copy link
Member Author

I wanted to have a look at some others improvements to the date module but we can do it incrementally indeed.

I will revisit this PR at the beginning of next week, I need to update the changelog.

@MangelMaxime
Copy link
Member Author

@dbrattli I didn't forget about this PR.

While checking it I discovered that their was some errors in our implementation.

For example, time-only date in F# are using the now as the reference while in Python with the current implementation it is using 1900-01-01.

I have a fix for that and trying to fix a few others tests that are failing.

- Fix toString with known format
- Add supports for DateTimeKind
- Add supports for Ticks related methods
def today() -> datetime:
return datetime.replace(now(), hour=0, minute=0, second=0, microsecond=0)


def to_local_time(date: datetime) -> datetime:
return date.astimezone()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm worried about this one compared to #3691 since I think you will get problems when localtime is the same timezone as utc (+00). This is why I also removed the tzinfo in my PR

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the range of the comment is off ?

But yes, there is something to think about here

@MangelMaxime
Copy link
Member Author

@dbrattli I believe you are correct when pointing out that my proposition for supporting DateTimeKind has some limitations.

The difficulty is that in .NET we want to represents 3 states:

  • DateTimeKind.Local
  • DateTimeKind.Utc
  • DateTimeKind.Unspecified

If I try to summarise our current 2 propositions:

Your implementation My implementation
DateTimeKind.Local date.tzinfo is None ⚠️ date.tzinfo is not None and date.tzinfo != timezone.utc
DateTimeKind.Utc date.tzinfo == timezone.utc ⚠️ date.tzinfo == timezone.utc
DateTimeKind.Unspecified date.tzinfo is None

In my case, I added ⚠️ because like you mentioned if your local timezone is UTC then we are in grey area because I don't know if we should return DateTimeKind.Local or DateTimeKind.UTC.

Because of that, I think if we want to stick with using native datetime to represents DateTime we will need to take a decision on which limitations we want.

  1. We say that for Python dates are always treated as local unless DateTimeKind.Utc is passed at its creation (or when
    setting it). ==> Leads to your implementation

    • Should we allow creating date without specifying DateTimeKind in Python?
    • Should we generate errors if the user tries to use DateTimeKind.Unspecified?
  2. We wants to stick "closer" (sorry for the lack of another word...) to .NET behaviour and decide that for all timezones we have the same behaviour but if the user is located in UTC timezone then we always report it as DateTimeKind.UTC.

Do you think my analysis is correct ? Which solutions do you prefer?

The others solutions I see are:

  1. Find a way to attach kind property to the datetime type but I don't think Python allows that
  2. Wrap datetime inside of a custom class so we can have our kind property

I will try to give a try to implements DateTimeOffset because I believe some of the question we currently have will also occurs for it. Hopefully, it will be less difficult because DateTimeOffset always needs the offset except for 1 constructor

CleanShot 2024-01-07 at 11 14 50@2x

@ncave
Copy link
Collaborator

ncave commented Jan 7, 2024

@MangelMaxime

The others solutions I see are:
1) Find a way to attach kind property to the datetime type but I don't think Python allows that
2) Wrap datetime inside of a custom class so we can have our kind property

Both are viable options. For DateTime, JS is using #1 and Rust is using #2 (i.e. custom class with kind property).

@MangelMaxime
Copy link
Member Author

@dbrattli This is finally ready for review 🎉, I think I covered most of DateTime features and if need I believe this is a good foundation to add more API over time if needed.

Compatibility

To be added to Fable website for reference

Because Fable doesn't support Globalisation,DateTime.ToString behave has if CultureInfo.InvariantCulture is passed.

Python datetime is precise up to 6 digit microseconds meaning that we don't support fffffff or FFFFFFF custom format.

Similarly when parsing an ISO date format, it is possible that there is a lost of precision for the microseconds value. Especially for Python 3.10, the date will be adapted to use exactly 6 digits for the microseconds value. Python 3.10 doesn't support ISO date format out of the box.

  • 2009-06-15T13:45:30.6175425 is transformed to 2009-06-15T13:45:30.617542
  • 2009-06-15T13:45:30.05 is transformed to 2009-06-15T13:45:30.050000
    -2009-06-15T13:45:30. is transformed to 2009-06-15T13:45:30.000000

Because of number precision it is possible for Ticks to differs a little from .NET value.

DateTime(1, 1, 1, 0, 0, 0, 0, 99, DateTimeKind.Utc).Ticks should be 990 but Python returns 1040
because d.timestamp() returns -62135596799.9999 instead of -62135596800000.

To fix that we would need to compute our own timestamp value but I am not sure if we could do better. If someone wants to give it a try later they can contribute via a PR.

@MangelMaxime MangelMaxime requested a review from dbrattli January 22, 2024 19:59
@dbrattli
Copy link
Collaborator

Looks great. This is really fantastic work @MangelMaxime!! Just a few comments and suggestions. PS: don't forget to change the title (remove WIP) before merging.

Copy link
Collaborator

@dbrattli dbrattli left a comment

Choose a reason for hiding this comment

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

Great work!!

@MangelMaxime MangelMaxime changed the title [WIP] Work on DateTime and DateTimeOffset for Python Work on DateTime and DateTimeOffset for Python Jan 23, 2024
@MangelMaxime MangelMaxime changed the title Work on DateTime and DateTimeOffset for Python Work on DateTime for Python Jan 23, 2024
@MangelMaxime
Copy link
Member Author

Thank you and thanks for reviewing the PR over time.

@MangelMaxime MangelMaxime merged commit a1434ff into main Jan 23, 2024
17 checks passed
@MangelMaxime MangelMaxime deleted the feature/date_parse branch January 23, 2024 19:20
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