feat: Add support for custom timestamp format with time_format()#6
Conversation
arferreira
left a comment
arferreira
left a comment
There was a problem hiding this comment.
Good first contribution, clean and focused! Just left few things before merging could improve more.
src/logger.rs
Outdated
| colorize: bool, | ||
| fields: Vec<(String, String)>, | ||
| formatter: Box<dyn Formatter>, | ||
| time_format: Option<&'static str>, |
There was a problem hiding this comment.
I think we could store it as String
There was a problem hiding this comment.
If you only change the variable type, it will not have much impact. However, if you change the parameter of the time_format() method to String, it will accept both string values and literals, making the method more flexible. Would you like to make this change for greater flexibility, or keep the current approach since changing only the variable type does not affect functionality?
There was a problem hiding this comment.
It doesn't affect functionality at all, but lt's unnecessarily restrictive and breaks consistency, because every other string-accepting builder method is using String.
There was a problem hiding this comment.
I understand. So, I will change the method parameter to &str, as it follows the pattern used in the project.
src/logger.rs
Outdated
| } | ||
|
|
||
| pub fn time_format(mut self, time_format: &'static str) -> Self { | ||
| self.time_format = Some(time_format); |
There was a problem hiding this comment.
thus we have to convert .to_string()
src/logger.rs
Outdated
| match &self.time_format { | ||
| Some(fmt) => Some(Local::now().format(fmt).to_string()), | ||
| None => Some(Local::now().format("%H:%M:%S").to_string()), | ||
| } |
There was a problem hiding this comment.
you can make it simpler:
let fmt = self.time_format.as_deref().unwrap_or("%H:%M:%S");
Some(Local::now().format(fmt).to_string())There was a problem hiding this comment.
This is a good and idiomatic
There was a problem hiding this comment.
ixe o cara ta trabalhando pra gringa agora
|
BTW, I think sheen would shine on Rapina's, feel free if you want to bring for us |
3 participants
Description
This PR adds support for a custom timestamp format using the .time_format() builder method, as requested in issue #2.
I chose to implement this as a separate method, so users can enable or disable the timestamp with .timestamp(true/false) and then optionally set the format with .time_format("...").
This approach keeps the API flexible and clear, following the builder pattern.
I considered passing the format directly to .timestamp(), but separating the concerns makes the configuration more intuitive and easier to maintain.
Closes #2
Type of Change
Checklist
cargo fmtrancargo clippypassescargo testpasses