feat(fs): expose the metadata for fs services #5005
Conversation
Xuanwo
changed the title
core/src/services/fs/lister.rs
Outdated
| get_metadata(self.op.metakey(), Some(fs_meta), None)? | ||
| }; | ||
|
|
||
| let p = if is_dir { |
There was a problem hiding this comment.
We don't need is_dir, we can use metadata.mode() to check.
core/src/services/fs/lister.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn get_metadata( |
There was a problem hiding this comment.
Hi, I feel like this helper function is a bit overdesigned. How about writing the logic directly? OpenDAL doesn't have a code style yet, but we tend to optimize for readability and avoid unnecessary abstraction, which can help a lot.
There was a problem hiding this comment.
I meant to share the same logic in the List and BlockingList
There was a problem hiding this comment.
I meant to share the same logic in the
ListandBlockingList
Yes, I understand your motivation. However, sharing code that is used only twice is not encouraged in OpenDAL.
There was a problem hiding this comment.
Get it, will follow your suggestion, thanks
core/src/services/fs/lister.rs
Outdated
| fs_meta: Option<std::fs::Metadata>, | ||
| file_type: Option<FileType>, | ||
| ) -> Result<(Metadata, bool)> { | ||
| let ft = if let Some(t) = fs_meta.as_ref() { |
There was a problem hiding this comment.
If we expand logic, we don't need this.
core/src/services/fs/lister.rs
Outdated
| let contains_metakey = |k| metakey.contains(k) || metakey.contains(Metakey::Complete); | ||
| let fs_meta = fs_meta.unwrap(); | ||
|
|
||
| if contains_metakey(Metakey::ContentLength) { |
There was a problem hiding this comment.
We don't need to check the meta key. If we have sent metadata syscall, we can fill in any known data.
| @@ -93,17 +116,38 @@ impl oio::BlockingList for FsLister<std::fs::ReadDir> { | |||
| // (no extra system calls needed), but some Unix platforms may | |||
| // require the equivalent call to symlink_metadata to learn about | |||
| // the target file type. | |||
| let file_type = de.file_type().map_err(new_std_io_error)?; | |||
| let default_meta = self.op.metakey() == Metakey::Mode; | |||
There was a problem hiding this comment.
Hi, please remove the upper comment before de.file_type().
Which issue does this PR close?
Part of #4746.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?