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

Add Optional Support #3303

Merged
merged 3 commits into from
Feb 25, 2023
Merged

Add Optional Support #3303

merged 3 commits into from
Feb 25, 2023

Conversation

tom-huntington
Copy link
Contributor

@tom-huntington tom-huntington commented Feb 11, 2023

A formatter for std::optional as per #1367 (comment)

The code is just copied from barry's slideware where the specifier just get parsed through to the formatter of the underling type.

As such there is no customization of printing of the optional's container for which I originally went with the opinionated Some(...) and None, but then changed to Option(...) and Empty. I'm not very convinced about nullopt when you're trying to study complicated output:

{(50258, 0, None), (50364, 0, None), (538, 0, Some(" by")), (428, 42, Some(" your")), (9827, 100, Some(" spell")), (11, 100, Some(",")), (457, 121, Some(" but")), (4374, 100, Some(" release")), (385, 142, Some(" me")), (490, 156, Some(" from")), (452, 168, Some(" my")), (13543, 177, Some(" bands")), (11, 260, Some(",")), (365, 264, Some(" with")), (264, 268, Some(" the")), (854, 270, Some(" help")), (295, 282, Some(" of")), (428, 290, Some(" your")), (665, 300, Some(" good")), (2377, 312, Some(" hands")), (13, 350, Some(".")), (26214, 374, Some(" Gentle")), (6045, 442, Some(" breath")), (50816, 452, None), (50816, 452, None), (295, 1223, Some(" of")), (6342, 487, Some(" yours")), ...

{(50258, 0, Empty), (50364, 0, Empty), (538, 0, Option(" by")), (428, 42, Option(" your")), (9827, 100, Option(" spell")), (11, 100, Option(",")), (457, 121, Option(" but")), (4374, 100, Option(" release")), (385, 142, Option(" me")), (490, 156, Option(" from")), (452, 168, Option(" my")), (13543, 177, Option(" bands")), (11, 260, Option(",")), (365, 264, Option(" with")), (264, 268, Option(" the")), (854, 270, Option(" help")), (295, 282, Option(" of")), (428, 290, Option(" your")), (665, 300, Option(" good")), (2377, 312, Option(" hands")), (13, 350, Option(".")), (26214, 374, Option(" Gentle")), (6045, 442, Option(" breath")), (50816, 452, Empty), (50816, 452, Empty), (295, 1223, Option(" of")), ...

@tom-huntington tom-huntington changed the title Add Optional Add Optional Support Feb 11, 2023
@tom-huntington
Copy link
Contributor Author

tom-huntington commented Feb 11, 2023

wchar_t formatting is now working for std::optional, although not for std::variant

include/fmt/std.h Outdated Show resolved Hide resolved
test/std-test.cc Outdated Show resolved Hide resolved
@tom-huntington tom-huntington force-pushed the optional branch 2 times, most recently from d377588 to 1a62615 Compare February 13, 2023 22:23
Copy link
Contributor

@vitaut vitaut 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 the PR. For consistency with other formatters such as variant I suggest replacing Option with optional and use lowercase empty (or none).

include/fmt/std.h Outdated Show resolved Hide resolved
include/fmt/std.h Outdated Show resolved Hide resolved
@tom-huntington
Copy link
Contributor Author

tom-huntington commented Feb 19, 2023

Thanks for the PR. For consistency with other formatters such as variant I suggest replacing Option with optional and use lowercase empty (or none).

@vitaut sure, I was just thinking you might, rather, want to change the variant formatter (despite capitalization going against the standard library's convention). For readability.

include/fmt/std.h Outdated Show resolved Hide resolved
include/fmt/std.h Outdated Show resolved Hide resolved
test/std-test.cc Outdated Show resolved Hide resolved
@vitaut vitaut merged commit 5b83020 into fmtlib:master Feb 25, 2023
@vitaut
Copy link
Contributor

vitaut commented Feb 25, 2023

Thank you!

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.

4 participants