Skip to content

BUG: Add AM/PM token support on guess_datetime_format #53244

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

Closed
wants to merge 5 commits into from

Conversation

juli4nb4dillo
Copy link
Contributor

@juli4nb4dillo juli4nb4dillo commented May 15, 2023

@juli4nb4dillo juli4nb4dillo changed the title Ampm token BUG: Add AM/PM token support on guess_datetime_format May 15, 2023
@MarcoGorelli MarcoGorelli self-requested a review May 15, 2023 20:37
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks for your PR - there's some failing tests, could you fix them up please?

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

thanks - will need a whatsnew note for 2.1 too

Comment on lines 211 to 212
("2005/11/09 10:15 AM", "%Y/%m/%d %I:%M %p"),
("2005/11/09 10:15:32 AM", "%Y/%m/%d %I:%M:%S %p"),
("Tue 24 Aug 2021 01:30:48 AM", "%a %d %b %Y %H:%M:%S %p"),
("Tuesday 24 Aug 2021 01:30:48 AM", "%A %d %b %Y %H:%M:%S %p"),
("Tue 24 Aug 2021 01:30:48", "%a %d %b %Y %H:%M:%S"),
("Tuesday 24 Aug 2021 01:30:48", "%A %d %b %Y %H:%M:%S"),
Copy link
Member

Choose a reason for hiding this comment

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

can you keep these in but change the expected fmt (the second argument) to be None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, let me see if I can setup my env to build and test locally, otherwise I'm kinda flying blind waiting for Github Actions to run

Copy link
Member

Choose a reason for hiding this comment

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

you could try gitpod if you're having trouble, else feel free to ask on the slack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm... it guesses the format to be "%a %d %b %Y %H:%M:%S AM" instead of None, which has to do with this loop:

try:
# If the token is numeric, then we likely didn't parse it
# properly, so our guess is wrong
float(tokens[i])
return None
except ValueError:
pass

Let me try again from the top, I think we can make it work with the "AM/PM" tokens easier now that I can test locally.

@mroeschke
Copy link
Member

Is this PR or #53257 supposed to fix this?

@juli4nb4dillo
Copy link
Contributor Author

@mroeschke It's duplicate, sorry for that, I wanted to test it from the top so I made another branch.
This one is wrong anyway.

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.

BUG: to_datetime ignores "AM" and "PM" unless format is explicitly set
3 participants