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 available range of datetime for Local.from_utc_datetime and Local.from_local_datetime functions #979

Closed
wants to merge 2 commits into from

Conversation

vvkostenko
Copy link

fixes #977

@djc
Copy link
Contributor

djc commented Mar 13, 2023

Hmm, I kind of don't want to take this because the windows module code is just so crappy? I think we should almost completely scrap the windows module and rebuild it from first principles to implement the now() and naive_to_local() functions with minimal use of the Windows API (and definitely avoiding all the Unixisms that are there now, like Timespace, Tm and a lot of strange indirection. Should probably also avoid the panic!() in call() in favor of yielding LocalResult::None.

Would you be interested in working on that? See also #989 which fixes other silliness in windows.

@nekevss
Copy link
Contributor

nekevss commented Mar 13, 2023

I don't know if it's exactly the way to go, but I did get a little OCD over the weekend after removing the panics and tried to clean up the module on this branch. Not sure if it's exactly the direction to go as it is mostly just cleaning up the current module and creating wrappers around windows_sys.

@djc
Copy link
Contributor

djc commented Mar 14, 2023

A bit of OCD is probably a good thing for this! Your branch seems to still maintain the Timespec and Tm which I think are complete distractions from the task at hand. Also your branch is targeting the main branch which uses windows-sys, but ideally we'd first land this on 0.4.x which still uses winapi (and then merge a similar change that uses windows-sys to main later). IMO the goal is to make the windows module as concise as possible, translating now() and naive_to_local() as directly as possible to Win32 APIs.

Copy link
Contributor

@jtmoon79 jtmoon79 left a comment

Choose a reason for hiding this comment

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

More tests is good.

@djc
Copy link
Contributor

djc commented Apr 5, 2023

I think the changes here have been addressed by #992?

@jtmoon79
Copy link
Contributor

jtmoon79 commented Apr 5, 2023

I think the changes here have been addressed by #992?

Looks like @nekevss covered this in #992 (including the test_from_naive_date_time_windows). Let's defer to that one. This PR should be Closed.

@djc djc closed this Apr 5, 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.

None yet

4 participants