Replaced str with Path
#1919
Replaced str with Path
#1919
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1919 +/- ##
==========================================
- Coverage 85.44% 85.04% -0.41%
==========================================
Files 788 793 +5
Lines 92483 94600 +2117
==========================================
+ Hits 79021 80449 +1428
- Misses 13462 14151 +689 ☔ View full report in Codecov by Sentry. |
| } | ||
|
|
||
| impl FileApplicationLoggerInstaller { | ||
| /// Create a new file application logger. | ||
| pub fn new(path: &str) -> Self { | ||
| pub fn new(path: impl AsRef<Path>) -> Self { |
There was a problem hiding this comment.
Is it better to have AsRef<Path> or Into<PatBuf>? If &str also implements Into<PathBuf> I think it might be more flexible, otherwise maybe AsRef<Path> is better.
There was a problem hiding this comment.
That's a good question. I don't know which one is better. In this situation I think either one will work just fine.
There was a problem hiding this comment.
It depends on the path usage, if a path must be only read then AsRef<Path> would be better, otherwise a Into<PathBuf> is preferred. We can have a look at how a path is treated internally in my opinion
There was a problem hiding this comment.
I noticed that this PR is still open. Are there any changes you would like me to make before merging it?
There was a problem hiding this comment.
We should use AsRef as the default choice for most file system operations in the codebase. This provides flexibility and efficiency for reading paths. We should only use Into<PathBuf> when we specifically need to own or modify the path data.
|
Thanks for fixing. Once the CI testing is fixed we can merge |
|
Merged. Thank you very much for your contribution! |
Checklist
run-checks allscript has been executed.Changes
In Rust, whenever dealing with the file system, it is recommended to use the
PathandPathBufstructs instead ofstrandString. In theburncode though,stris often used to describe a file path or a directory. This PR changes these instances to usePath.Testing
Since
strcan be safely converted toPath, this is a drop-in replacement for the previous code. Aside of running./run-checks, I've also tested it on an external project.