Skip to content

Fixed Period and PeriodIndex formatting #46361

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Mar 18, 2022
Merged

Fixed Period and PeriodIndex formatting #46361

merged 17 commits into from
Mar 18, 2022

Conversation

smarie
Copy link
Contributor

@smarie smarie commented Mar 14, 2022 •
edited
Loading

Note: this is the first PR extracted from #46116. More to come soon :)

smarie
smarie commented Mar 14, 2022
View reviewed changes
smarie
smarie commented Mar 14, 2022
View reviewed changes
pandas/core/arrays/datetimelike.py
Comment on lines +1592 to +1595
Formats supported by the C `strftime` API but not by the python string format
doc (such as `"%%R"`, `"%%r"`) are not officially supported and should be
preferably replaced with their supported equivalents (such as `"%%H:%%M"`,
`"%%I:%%M:%%S %%p"`).
Copy link
Contributor Author

@smarie smarie Mar 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This clarification comes from the fact that on one side we refer to python strftime doc (just above this text), while in the example (line 1623 below) the %r directive is used !

I checked: neither %r or %R are in the official python doc for strftime, and they are not as well in the extended directives that we support in Period.strftime. But... they work. So this needs explicit clarification I think.

@smarie smarie mentioned this pull request Mar 14, 2022
Closed
3 tasks
@smarie
Copy link
Contributor Author

smarie commented Mar 14, 2022

The error in CI seems related to usage of assert_produces_warning. What is strange is that it worked in #46116 .. were there any change on main related to this that could explain the issue ?

@smarie
Copy link
Contributor Author

smarie commented Mar 15, 2022

Ready for review @jreback @mroeschke .

@jreback jreback added Output-Formatting __repr__ of pandas objects, to_string Period Period data type labels Mar 16, 2022
@jreback jreback added this to the 1.5 milestone Mar 16, 2022
@smarie
Copy link
Contributor Author

smarie commented Mar 16, 2022

Thanks @jbrockmendel and @mroeschke for the review !
I left one comment pending (parametrization) and all others were taken into account

@smarie
Copy link
Contributor Author

smarie commented Mar 17, 2022

All set (thanks @mroeschke for resolving the last comment).
The last failing mypy check seems unrelated (pandas/io/clipboard/init.py:97: error: Module has no attribute "WinError" [attr-defined])

@mroeschke
Copy link
Member

Could you merge main in one more time just to be sure?

@jreback jreback merged commit 5af2229 into pandas-dev:main Mar 18, 2022
@jreback
Copy link
Contributor

jreback commented Mar 18, 2022

thanks @smarie very nice!

@smarie
Copy link
Contributor Author

smarie commented Mar 18, 2022

Thank you all for the quick review !

@smarie smarie deleted the feature/46252_period_formatting branch March 18, 2022 10:56
yehoshuadimarsky pushed a commit to yehoshuadimarsky/pandas that referenced this pull request Jul 13, 2022
@smarie @yehoshuadimarsky
Fixed Period and PeriodIndex formatting (pandas-dev#46361)
09f7ad5
@smarie smarie mentioned this pull request Dec 12, 2022
Closed
5 tasks
@smarie smarie mentioned this pull request May 14, 2023
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbrockmendel jbrockmendel jbrockmendel left review comments

@mroeschke mroeschke mroeschke approved these changes

Assignees
No one assigned
Labels
Output-Formatting __repr__ of pandas objects, to_string Period Period data type
Projects
None yet
Milestone
1.5
Development

Successfully merging this pull request may close these issues.

BUG: Period formatting directive %l (millisecond) and %u (microseconds) provide wrong results
4 participants
@smarie @mroeschke @jreback @jbrockmendel