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

Allow overriding need_localtime for custom formatter #2365

Merged
merged 3 commits into from
May 7, 2022

Conversation

conr2d
Copy link
Contributor

@conr2d conr2d commented May 7, 2022

If user wants to use the pattern with no builtin flags related to time, but custom formatter that needs time information, there is no way to make pattern_formatter pass the updated const std::tm& to format() of custom formatter. This PR allows user to override need_localtime so that custom formatter can utilize cached_tm with the pattern not enabling need_localtime by default.

@gabime
Copy link
Owner

gabime commented May 7, 2022

Thanks. Need to fid the clone() function ad well. Any idea why tests fail?

memory_buf_t formatted;
spdlog::details::log_msg msg(spdlog::source_loc{}, "logger-name", spdlog::level::info, "some message");
formatter->format(msg, formatted);
REQUIRE(to_string_view(formatted) == oss.str());
Copy link
Owner

@gabime gabime May 7, 2022

Choose a reason for hiding this comment

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

1.This can fail if the time changes when getting here
2. This fails on windows (eol is “\r\n”)

@conr2d
Copy link
Contributor Author

conr2d commented May 7, 2022

Thank you for fast feedback.

  • I added copying the value of need_localtime to pattern_formatter::clone().
  • Test failed due to eol symbol. I guess that automated test for this repository is processed on WIN32 environment, and eol would be \r\n instead of \n. I set eol for the pattern formatter in test to \n explicitly.
  • As you mentioned, test can fail with low probability if the expected string and actually formatted string are generated in different seconds. I followed the one case of original tests, date MM/DD/YY, but if you think the tests should be fixed in both cases, I will investigate how to fix the time to refer.

@gabime gabime merged commit 9b4b373 into gabime:v1.x May 7, 2022
@gabime
Copy link
Owner

gabime commented May 7, 2022

Thanks @conr2d . Merged.
If you could investigate on fixing the formatter date tests it would be great.

@conr2d conr2d deleted the feature/need_localtime branch May 8, 2022 03:10
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.

None yet

2 participants