Add option with_short_file_names #13
Conversation
src/format.rs
Outdated
| .map(|f| { | ||
| if self.with_short_file_names { | ||
| f.rsplit_once('/') | ||
| .map(|(_, file_name)| file_name) | ||
| .unwrap_or_default() | ||
| } else { | ||
| f | ||
| } |
There was a problem hiding this comment.
I'm not opposed to this change, but I'd prefer an approach like this instead:
| .map(|f| { | |
| if self.with_short_file_names { | |
| f.rsplit_once('/') | |
| .map(|(_, file_name)| file_name) | |
| .unwrap_or_default() | |
| } else { | |
| f | |
| } | |
| .map(|f| { | |
| if self.with_short_file_names { | |
| let f = Path::new(f); | |
| f.file_name().map(OsStr::to_str); | |
| } else { | |
| f | |
| } |
I haven't compiled this, so it might fail, but I'd rather be operating in terms of Paths instead of string manipulation.
src/lib.rs
Outdated
| @@ -185,6 +187,13 @@ impl<T> Glog<T> { | |||
| } | |||
| } | |||
|
|
|||
| pub fn with_short_file_names(self, with_short_file_names: bool) -> Glog<T> { | |||
There was a problem hiding this comment.
I understand that this behavior is meant to match glog's, but I can see "short" being somewhat ambiguous (e.g., what qualifies as "short"? A single directory preceding it?).
How would you feel about renaming this to with_trimmed_directory and maybe adding a seperate builder option that exposes Path::strip_prefix, allowing for more fine-grained rendering of paths?
There was a problem hiding this comment.
You're right, I'll use Path and rename.
Not sure about using Path::strip_prefix. Could be useful, though with the module shown (with_target(true)) we would never use it. Either the full path or just the filename, I doubt that stripping away just a part makes much sense.
There was a problem hiding this comment.
For Rust there's a very high likelihood that the source will be called lib.rs (or maybe mod.rs) and even with one directory element you'll get src/lib.rs. Are you sure that this change will actually give you useful logs?
There was a problem hiding this comment.
In combination with with_target(true), otherwise not.
This option strips the directory from the file path, like some other glog libraries do by default.
|
I renamed the option and used Path to strip the directory. |
src/format.rs
Outdated
| .unwrap_or_default() | ||
| .unwrap_or_default() |
There was a problem hiding this comment.
Wouldn't it be better to use the original full filename if something goes wrong here?
There was a problem hiding this comment.
Yes that is better. I added a fixup that does this.
This option strips a prefix from the target file path. If the prefix doesn't match, the full path is logged.
|
I also added I added a commit on a test branch, with a little example that shows the resulting logs of various cases: |
|
Sorry, I should've mentioned this last week when I first saw this PR, but I only thought of this approach last night. How would you feel about a trait called If you're interested, I can make a PR with the above? |
Sure, you modify this as you wish. |
Hello, we use very long paths.
Our other glog libraries (go and c++) log only the file name and not the full path.
I'd like to have the same for our rust logs, so I made this small addition.