PDEP0004: implementation by MarcoGorelli · Pull Request #49024 · pandas-dev/pandas (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation118 Commits54 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Choose a reason for hiding this comment

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

thanks for moving this along

MarcoGorelli added 8 commits

October 20, 2022 15:29

Marco Gorelli and others added 4 commits

October 21, 2022 19:34

Choose a reason for hiding this comment

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

LGTM


pd.to_datetime(["04-01-2012 10:00"], dayfirst=True)

pd.to_datetime(["14-01-2012", "01-14-2012"], dayfirst=True)
pd.to_datetime(["04-14-2012 10:00"], dayfirst=True)

Choose a reason for hiding this comment

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

is this line still necessary?

Choose a reason for hiding this comment

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

yeah it's an example demonstrating that dayfirst isn't strict

but it's good you've highlighted this, as the blank line I'd removed was preventing it from rendering properly. now it looks fine:

Choose a reason for hiding this comment

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

@jbrockmendel thanks for your review - any other objections? Sorry to tag, just hoping to move this forwards before more merge conflicts arise

Choose a reason for hiding this comment

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

all good here. ill take another look tomorrowish if this is still up, but if it is merged before then i wont complain

Choose a reason for hiding this comment

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

I think this looks good. One question I might just be blanking on otherwise happy to merge

Choose a reason for hiding this comment

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

I say ship it

just a couple of comments

MarcoGorelli added 2 commits

December 9, 2022 08:42

Choose a reason for hiding this comment

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

Thanks @jreback ! I've responded

The Py311-dev builds are failing because of #50124

Thanks all! @Dr-Irv there's still a "requested changes" from you - any further comments?

MarcoGorelli added 4 commits

December 12, 2022 16:51

Choose a reason for hiding this comment

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

I'm good now - all issues resolved.

I'm good now - all issues resolved.

Thanks! Really appreciate all the reviews here

That's 4 explicit approvals from core members, unless there's objections I'll merge soonish then - there's already 110+ messages here and it's getting hard to navigate

If minor objections come up I'll address them in a follow-up, and if there's any major ones we can always revert

MarcoGorelli and others added 4 commits

December 13, 2022 08:23

Reviewers

noatamir noatamir left review comments