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

Expose format_into and format_into_io for DelayedFormat #1653

Closed
wants to merge 2 commits into from

Conversation

tugtugtug
Copy link

Request for #1649

  • renamed format to format_into and made it public.
  • added format_into_io to allow format into a std::io::Write.
  • added unittests
  • added benchmarks for the 3 methods, Display, format_into, and format_into_io.

Thanks for contributing to chrono!

If your feature is semver-compatible, please target the main branch;
for semver-incompatible changes, please target the 0.5.x branch.

Please consider adding a test to ensure your bug fix/feature will not break in the future.

Copy link

codecov bot commented Jan 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.03%. Comparing base (5f2f5a8) to head (b93c9af).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1653      +/-   ##
==========================================
+ Coverage   91.01%   91.03%   +0.01%     
==========================================
  Files          37       37              
  Lines       17342    17376      +34     
==========================================
+ Hits        15784    15818      +34     
  Misses       1558     1558              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Request for chronotope#1649
- renamed format to format_into and made it public.
- added format_into_io to allow format into a `std::io::Write`.
- added unittests
- added benchmarks for the 3 methods, `Display`, `format_into`, and
  `format_into_io`.
Comment on lines 100 to 118
/// Formats `DelayedFormat` into an `std::io::Write` instance.
/// # Errors
/// This function returns an error if formatting into the `std::io::Write` instance fails.
#[cfg(feature = "std")]
pub fn format_into_io(self, w: &mut impl std::io::Write) -> fmt::Result {
// wrapper to allow reuse of the existing string based
// writers
struct IoWriter<W: std::io::Write> {
writer: W,
}
impl<W: std::io::Write> fmt::Write for IoWriter<W> {
#[inline]
fn write_str(&mut self, s: &str) -> fmt::Result {
self.writer.write_all(s.as_bytes()).map_err(|_| fmt::Error)
}
}
let mut writer = IoWriter { writer: w };
self.format_into(&mut writer)
}
Copy link
Member

Choose a reason for hiding this comment

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

Not really interested in taking this. Callers can construct their own adapter if that is important to them.

Copy link
Author

Choose a reason for hiding this comment

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

made it into an example instead.

/// Formats `DelayedFormat` into a `core::fmt::Write` instance.
/// # Errors
/// This function returns an error if formatting into the `core::fmt::Write` instance fails.
pub fn format_into(&self, w: &mut impl Write) -> fmt::Result {
Copy link
Member

Choose a reason for hiding this comment

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

Suggest we call this write_to().

Copy link
Author

Choose a reason for hiding this comment

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

done.

Request for chronotope#1649
- renamed format to write-to and made it public.
- added unittests
- added benchmarks to compare and show  `Display` is slower than `write_to` in that
  specific use case.
@tugtugtug
Copy link
Author

created new pr #1654 with the changes recommended.

@tugtugtug tugtugtug closed this Jan 22, 2025
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.

2 participants