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

Conversation8 Commits6 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 }})

xref #30757 should go in before this because it contains the tests. After this, we'll be able to de-duplicate the two methods.

Hello @jbrockmendel! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-01-08 18:34:47 UTC

Choose a reason for hiding this comment

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

lgtm. TypeError makes more sense than ValueError where changed I think

Will need an API-breaking whatsnew for changing the type of the exception.

Though to confirm: TypeErrors are typically raised when the argument has an incorrect type. In this case, the type looks correct, since it's a Timestamp. It's a tz-naive timestamp though, so I wouldn't expect a ValueError.

if fill_val.tz:
with pytest.raises(ValueError, match=msg):
msg = "Cannot compare tz-naive and tz-aware"

Choose a reason for hiding this comment

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

@TomAugspurger is this what you are talking about? this I think was just wrong before, not an API change.

Choose a reason for hiding this comment

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

Yeah. I see now that we raise TypeError for Timestamp == Timestamp, which I think is incorrect but not worth changing.