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 TimePadding with Right and AddZeros #134

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/config.rs
Expand Up @@ -58,6 +58,17 @@ pub(crate) enum TimeFormat {
Custom(&'static [time::format_description::FormatItem<'static>]),
}

#[derive(Debug, Clone, Copy)]
/// Padding to be used for logging the time
pub enum TimePadding {
/// Add spaces on the right side, up to usize many
Right(usize),
/// Do not pad the time
Off,
/// If appliciable, add zeros to the subseconds
AddZeros
}

/// Configuration for the Loggers
///
/// All loggers print the message in the following form:
Expand All @@ -82,6 +93,7 @@ pub struct Config {
pub(crate) module: LevelFilter,
pub(crate) time_format: TimeFormat,
pub(crate) time_offset: UtcOffset,
pub(crate) time_padding: TimePadding,
pub(crate) filter_allow: Cow<'static, [Cow<'static, str>]>,
pub(crate) filter_ignore: Cow<'static, [Cow<'static, str>]>,
#[cfg(feature = "termcolor")]
Expand Down Expand Up @@ -226,6 +238,12 @@ impl ConfigBuilder {
self
}

/// Set how the time should be padded
pub fn set_time_padding(&mut self, padding: TimePadding) -> &mut ConfigBuilder {
self.0.time_padding = padding;
self
}

/// Sets the offset used to the current local time offset
/// (overriding values previously set by [`ConfigBuilder::set_time_offset`]).
///
Expand Down Expand Up @@ -345,6 +363,7 @@ impl Default for Config {
module: LevelFilter::Off,
time_format: TimeFormat::Custom(format_description!("[hour]:[minute]:[second]")),
time_offset: UtcOffset::UTC,
time_padding: TimePadding::AddZeros,
Copy link
Owner

Choose a reason for hiding this comment

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

I think this modifies the current default (of no padding), which I would want to release as a minor version. Can we maybe consider this to be Off by default?

Choose a reason for hiding this comment

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

Leaving the padding as Off is certainly possible, but I would argue that setting a padding by default would improve usability a little.

Copy link
Owner

Choose a reason for hiding this comment

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

I agree, but then we will need a new major version.

filter_allow: Cow::Borrowed(&[]),
filter_ignore: Cow::Borrowed(&[]),
write_log_enable_colors: false,
Expand Down
35 changes: 24 additions & 11 deletions src/loggers/logging.rs
@@ -1,4 +1,4 @@
use crate::config::{TargetPadding, TimeFormat};
use crate::config::{TargetPadding, TimeFormat, TimePadding};
use crate::{Config, LevelPadding, ThreadLogMode, ThreadPadding};
use log::{LevelFilter, Record};
use std::io::{Error, Write};
Expand Down Expand Up @@ -72,22 +72,35 @@ pub fn write_time<W>(write: &mut W, config: &Config) -> Result<(), Error>
where
W: Write + Sized,
{
use time::error::Format;
use time::format_description::well_known::*;

let time = time::OffsetDateTime::now_utc().to_offset(config.time_offset);
let res = match config.time_format {
TimeFormat::Rfc2822 => time.format_into(write, &Rfc2822),
TimeFormat::Rfc3339 => time.format_into(write, &Rfc3339),
TimeFormat::Custom(format) => time.format_into(write, &format),
let formatted = match config.time_format {
TimeFormat::Rfc2822 => time.format(&Rfc2822),
TimeFormat::Rfc3339 => time.format(&Rfc3339),
TimeFormat::Custom(format) => time.format(&format),
};
match res {
Err(Format::StdIo(err)) => return Err(err),
Err(err) => panic!("Invalid time format: {}", err),
_ => {}
let mut formatted: String =
formatted.unwrap_or_else(|err| panic!("Invalid time format: {}", err));
Copy link
Owner

Choose a reason for hiding this comment

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

I think this drops the special casing for Format::StdIo, you should be able to use assign using the match here instead.

Suggested change
formatted.unwrap_or_else(|err| panic!("Invalid time format: {}", err));
match formatted {
Ok(res) => res,
Err(Format::StdIo(err)) => return Err(err),
Err(err) => panic!("Invalid time format: {}", err),
};


formatted = match config.time_padding {
TimePadding::Right(offset) => format!("{: <1$}", formatted, offset),
TimePadding::AddZeros => {
if matches!(config.time_format, TimeFormat::Rfc3339) {
// only appliciable with Rfc3339
//
// the rfc3339 timestamp is 30 characters long, if it would not end with a 0 in the
// subseconds part. To fix this inconsistency, we add zeros before the Z.
while formatted.len() < 30 {
Copy link
Author

Choose a reason for hiding this comment

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

Maybe make the 30 into a constant?

Copy link
Owner

Choose a reason for hiding this comment

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

Good idea

formatted.insert(formatted.len() - 1, '0')
}
Copy link
Owner

Choose a reason for hiding this comment

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

I feel like this is not very performant and there should be a way to not add the zeros one by one.

}
format!("{}", formatted)
}
TimePadding::Off => format!("{}", formatted),
};

write!(write, " ")?;
write!(write, "{} ", formatted)?;
Ok(())
}

Expand Down