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

Rework DateTime.Parse/TryParse and DateTimeOffset.Parse/TryParse #3654

Open
MangelMaxime opened this issue Dec 12, 2023 · 2 comments
Open

Rework DateTime.Parse/TryParse and DateTimeOffset.Parse/TryParse #3654

MangelMaxime opened this issue Dec 12, 2023 · 2 comments
Labels
python Python

Comments

@MangelMaxime
Copy link
Member

Description

Hello @dbrattli,

The last failing test I have in Thoth.Json currently are related to DateTime/DateTimeOffset and I work on a fix for that. While working on it, I discovered that the parse function is using dateutil which is not a Python standard API and so that function doesn't work.

def parse(string: str, detectUTC: bool = False) -> datetime:
from dateutil import parser
return parser.parse(string)

Currently, I am doing something really naive to use the native API and support at least a subset of the .NET formatting date:

def parse(string: str, detectUTC: bool = False) -> datetime:
    # from dateutil import parser

    try:
        return datetime.fromisoformat(string)
    except ValueError:
        try:
            return datetime.strptime(string, "%H:%M:%S")
        except ValueError:
            try:
                return datetime.strptime(string, "%I:%M:%S %p")
            except ValueError:
                # IMPORTANT: This format needs to be tested after %I:%M:%S %p
                # Otherwise, it happens that %H:%M:%S %p incorrectly parses some
                # of the string that can overlap with %I:%M:%S %p
                # Example: 1:5:34 PM
                try:
                    return datetime.strptime(string, "%H:%M:%S %p")
                except ValueError:
                    raise ValueError("Unsupported format by Fable")

Really ugly, I know ^^. This is just a temporary solution instead of something that crash rightnow.

If we want to use dateutil, we need to publish fable-library to PyPi (or whatever is the package repository in Python) and check if this doesn't create others issues like we discussed on Slack regarding the Rust distribution of the library.

Or embed the dateutil source code inside of Fable, like it has been done in the past for BigInt in JavaScript.

How do you envision the DateTime/DateTimeOffset parsing functions to be implemented?

@MangelMaxime MangelMaxime added the python Python label Dec 12, 2023
@dbrattli
Copy link
Collaborator

@MangelMaxime I would be happy if we could get rid of the dateutil dependency. Here's a copilot refactor of your code:

def parse(string: str, detectUTC: bool = False) -> datetime:
    formats = {
        # Matches a date-time string in the format "YYYY-MM-DDTHH:MM:SS"
        r"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}": "%Y-%m-%dT%H:%M:%S",

        # Matches a time string in 24-hour format "HH:MM:SS"
        r"\d{2}:\d{2}:\d{2}": "%H:%M:%S",

        # Matches a time string in 12-hour format with AM/PM "HH:MM:SS AM" or "HH:MM:SS PM"
        r"\d{2}:\d{2}:\d{2} [AP]M": "%I:%M:%S %p",
    }

    for pattern, format in formats.items():
        if re.fullmatch(pattern, string):
            return datetime.strptime(string, format)

    raise ValueError("Unsupported format by Fable")

@MangelMaxime
Copy link
Member Author

This looks cleaner and avoid the infinite nested in the code. I will give it a try thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Python
Projects
None yet
Development

No branches or pull requests

2 participants