-
Notifications
You must be signed in to change notification settings - Fork 73
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
make chrono optional #109
base: master
Are you sure you want to change the base?
make chrono optional #109
Conversation
Final chrono 0.4.20 is now out, any comments to the PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for taking so long to review this and thanks for working on it!
I have a few remarks, because I would like to avoid API breakage where possible and abstract more over the two implementations, so the library feels similar, independent of the enabled featuers.
src/config.rs
Outdated
@@ -79,7 +83,15 @@ pub struct Config { | |||
pub(crate) target: LevelFilter, | |||
pub(crate) target_padding: TargetPadding, | |||
pub(crate) location: LevelFilter, | |||
#[cfg(feature = "chrono")] | |||
pub(crate) time_format: Cow<'static, str>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we maybe not have totally different types for the two features?
As a suggestion we could just change the Custom
variant of the TimeFormat
enum to contain a 'static Cow, when chrono is enabled and still support Rfc2822 and Rfc3339 out of the box with both variants.
src/config.rs
Outdated
#[cfg(feature = "chrono")] | ||
pub(crate) time_format: Cow<'static, str>, | ||
#[cfg(feature = "chrono")] | ||
pub(crate) time_offset: FixedOffset, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with the offset. Maybe we could make this a light wrapper, that has a UTC
constant (like both time::UtcOffset
and chrono::FixedOffset
have) and a constructor from_offset(hours: i8)
or something? Depending on the feature enabled, that one could also implement either From<time::UtcOffset>
or From<chrono::FixedOffset>
.
#[cfg(feature = "chrono")] | ||
pub(crate) time_offset: FixedOffset, | ||
#[cfg(feature = "chrono")] | ||
pub(crate) time_local: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then only the time_local
field would be specific to chrono (and I don't really see a way to remove it).
@@ -191,6 +221,7 @@ impl ConfigBuilder { | |||
/// .set_time_format_custom(format_description!("[hour]:[minute]:[second].[subsecond]")) | |||
/// .build(); | |||
/// ``` | |||
#[cfg(not(feature = "chrono"))] | |||
pub fn set_time_format_custom( | |||
&mut self, | |||
time_format: &'static [FormatItem<'static>], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So instead of adding set_time_format
and set_time_format_str
, only this parameter could change to something like time_format: impl Into<Cow<'static, str>>
.
src/config.rs
Outdated
@@ -200,30 +231,48 @@ impl ConfigBuilder { | |||
} | |||
|
|||
/// Set time format string to use rfc2822. | |||
#[cfg(not(feature = "chrono"))] | |||
pub fn set_time_format_rfc2822(&mut self) -> &mut ConfigBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see this function being supported, when chrono is enabled as well. See comment for write_time
below.
src/config.rs
Outdated
pub fn set_time_format_rfc2822(&mut self) -> &mut ConfigBuilder { | ||
self.0.time_format = TimeFormat::Rfc2822; | ||
self | ||
} | ||
|
||
/// Set time format string to use rfc3339. | ||
#[cfg(not(feature = "chrono"))] | ||
pub fn set_time_format_rfc3339(&mut self) -> &mut ConfigBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as set_time_format_rfc2822
src/config.rs
Outdated
pub fn set_time_format_rfc3339(&mut self) -> &mut ConfigBuilder { | ||
self.0.time_format = TimeFormat::Rfc3339; | ||
self | ||
} | ||
|
||
/// Set offset used for logging time (default is 0) | ||
#[cfg(feature = "chrono")] | ||
pub fn set_time_offset(&mut self, time_offset: FixedOffset) -> &mut ConfigBuilder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per suggestion above this function would change it's parameter to something like impl Into<Offset>
(if Offset
is our wrapper type).
src/config.rs
Outdated
pub fn set_time_offset(&mut self, offset: UtcOffset) -> &mut ConfigBuilder { | ||
self.0.time_offset = offset; | ||
self | ||
} | ||
|
||
/// set if you log in local timezone or UTC (default is UTC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is possibly a little confusing, because users setting an offset would not expect UTC or the Local timezone and it sounds like these two options are exclusive.
Rather this should state, that it changes the relative timezone before applying the offset.
src/loggers/logging.rs
Outdated
pub fn write_time<W>(write: &mut W, config: &Config) -> Result<(), Error> | ||
where | ||
W: Write + Sized, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here we would need to match the time_format
instead and just hardcode &'static str
-format strings for Rfc2822 and Rfc3339. chrono directly supports Rfc3339 with %+
, but Rfc2822 would need to be constructed manually.
Hi @Drakulix, thank you for your suggestions! They really makes sense, but I'm not skilled enough to implement them. I started with: #[derive(Debug, Clone)]
pub(crate) enum TimeFormat {
Rfc2822,
Rfc3339,
#[cfg(feature = "chrono")]
Custom(Cow<'static, str>),
#[cfg(not(feature = "chrono"))]
Custom(&'static [time::format_description::FormatItem<'static>]),
} But then how to continue, I don't know. |
605dde1
to
70f4dcb
Compare
This pull request is not ready to merge, is more for discussion.
Chrono has two new maintainer and in the last months there is continues developments on it. The segfault on localtime_r is fixed and there is more improvements.
The reason for make chrono optional again is, that many project rely on it, it has
set_time_to_local
and it has better formatting options then time.