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 glibc ext for day of month and week of year #3976

Merged
merged 10 commits into from
May 30, 2024
Merged

Conversation

ZaheenJ
Copy link
Contributor

@ZaheenJ ZaheenJ commented May 24, 2024

Allows for "-", "_", and "0" glibc padding specifiers to be used with day of the month (%d) and week of the year (%W,%U,%V) specifiers. I quickly drafted this based on #3271 since I needed this for my Waybar, so tell me if something is wrong. Additionally, trivially support "%k" and "%l", which strftime supports as equivalents to "%_H" and "%_I" respectively.

@ZaheenJ
Copy link
Contributor Author

ZaheenJ commented May 24, 2024

For the failing lint checks, I can just fix that with clang-format. However I don't know how to fix the other failing checks. I had to change these lines to include the additional pad parameter for it to compile on my system, but it seems to make CI upset only for the C++20 mac and linux builds because it thinks the parameter is unused.

fmt/include/fmt/chrono.h

Lines 987 to 991 in 2627fa5

FMT_CONSTEXPR void on_dec0_week_of_year(numeric_system, pad_type pad) { unsupported(); }
FMT_CONSTEXPR void on_dec1_week_of_year(numeric_system, pad_type pad) { unsupported(); }
FMT_CONSTEXPR void on_iso_week_of_year(numeric_system, pad_type pad) { unsupported(); }
FMT_CONSTEXPR void on_day_of_year() { unsupported(); }
FMT_CONSTEXPR void on_day_of_month(numeric_system, pad_type pad) { unsupported(); }

fmt/include/fmt/chrono.h

Lines 1936 to 1939 in 2627fa5

void on_dec0_week_of_year(numeric_system, pad_type pad) {}
void on_dec1_week_of_year(numeric_system, pad_type pad) {}
void on_iso_week_of_year(numeric_system, pad_type pad) {}
void on_day_of_month(numeric_system, pad_type pad) {}

@tchaikov
Copy link
Contributor

@ZaheenJ in that particular case, you might need to take closer look at the proposed patch:

--- a/include/fmt/chrono.h
+++ b/include/fmt/chrono.h
@@ -984,11 +984,19 @@ template <typename Derived> struct null_chrono_spec_handler {
   FMT_CONSTEXPR void on_abbr_month() { unsupported(); }
   FMT_CONSTEXPR void on_full_month() { unsupported(); }
   FMT_CONSTEXPR void on_dec_month(numeric_system) { unsupported(); }
-  FMT_CONSTEXPR void on_dec0_week_of_year(numeric_system, pad_type pad) { unsupported(); }
-  FMT_CONSTEXPR void on_dec1_week_of_year(numeric_system, pad_type pad) { unsupported(); }
-  FMT_CONSTEXPR void on_iso_week_of_year(numeric_system, pad_type pad) { unsupported(); }
+  FMT_CONSTEXPR void on_dec0_week_of_year(numeric_system, pad_type pad) {
+    unsupported();
+  }
+  FMT_CONSTEXPR void on_dec1_week_of_year(numeric_system, pad_type pad) {
+    unsupported();
+  }
+  FMT_CONSTEXPR void on_iso_week_of_year(numeric_system, pad_type pad) {
+    unsupported();
+  }
   FMT_CONSTEXPR void on_day_of_year() { unsupported(); }
-  FMT_CONSTEXPR void on_day_of_month(numeric_system, pad_type pad) { unsupported(); }
+  FMT_CONSTEXPR void on_day_of_month(numeric_system, pad_type pad) {
+    unsupported();
+  }
   FMT_CONSTEXPR void on_24_hour(numeric_system) { unsupported(); }
   FMT_CONSTEXPR void on_12_hour(numeric_system) { unsupported(); }
   FMT_CONSTEXPR void on_minute(numeric_system) { unsupported(); }

the linter actually suggests just to wrap the line.

include/fmt/chrono.h Outdated Show resolved Hide resolved
include/fmt/chrono.h Outdated Show resolved Hide resolved
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! Overall looks good but please document the new specifiers in https://github.com/fmtlib/fmt/blob/master/doc/syntax.rst and split the addition of %k and %l into a separate PR.

@ZaheenJ
Copy link
Contributor Author

ZaheenJ commented May 27, 2024

I didn't realize that the documentation for the "_", "-" and "0" padding modifiers was not there, I thought it would've been added in the original glibc extension PR. I added that based off the strftime man page if that's okay. I also will open a separate PR for the new specifiers.

doc/syntax.rst Outdated Show resolved Hide resolved
@ZaheenJ ZaheenJ changed the title Add glibc ext for day of month and week of year and %k and %l specifiers Add glibc ext for day of month and week of year May 29, 2024
@vitaut
Copy link
Contributor

vitaut commented May 29, 2024

LGTM but please rebase and note that the syntax doc has been converted to Markdown (you can use pandoc to convert your changes).

@vitaut vitaut merged commit ca8eeb0 into fmtlib:master May 30, 2024
42 of 43 checks passed
@vitaut
Copy link
Contributor

vitaut commented May 30, 2024

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