feat(bindings/python): Add repr for metadata#5783
Conversation
… better print and debug Signed-off-by: yihong0618 <[email protected]>
github-actions
bot
added
the
releases-note/refactor
Zheaoli
left a comment
Zheaoli
left a comment
There was a problem hiding this comment.
Would you mind adding unit test here. I think some simple assert is enough
Signed-off-by: yihong0618 <[email protected]>
Thanks and Done |
bindings/python/src/metadata.rs
Outdated
|
|
||
| pub fn __repr__(&self) -> String { | ||
| format!( | ||
| "Metadata(content_disposition={}, content_length={}, content_md5={}, content_type={}, etag={}, mode={})", |
There was a problem hiding this comment.
content_disposition might be empty most of the time, so I don't think it's worth printing in repr. How about this instead:
"Metadata(mode={}, content_length={}, content_type={}, etag={})"
We arrange the metadata by importance and ignore less frequently used metadata.
There was a problem hiding this comment.
cool will drop this, you are right.
and FYI last_modified maybe needed here too?
There was a problem hiding this comment.
Cool will update this patch after last_modified Done
There was a problem hiding this comment.
Done and lerned that, thanks
Signed-off-by: yihong0618 <[email protected]>
Signed-off-by: yihong0618 <[email protected]>
bindings/python/src/metadata.rs
Outdated
| let rfc3339 = dt.to_rfc3339_opts(SecondsFormat::Micros, false); | ||
| if let Some(pos) = rfc3339.find('.') { | ||
| let (base, tz) = rfc3339.split_at(pos + 1); | ||
| let tz_pos = tz.find('+').or_else(|| tz.find('-')).unwrap_or(tz.len()); | ||
| let (micros, tz_suffix) = tz.split_at(tz_pos); | ||
|
|
||
| let micros = format!("{:0<6}", micros); | ||
| format!("{}{}{}", base, micros, tz_suffix) | ||
| } else { | ||
| rfc3339 | ||
| } |
There was a problem hiding this comment.
Print last_modified in this way doesn't look good to me.
If we have a clear target format, maybe we can use DateTime::format instead.
bindings/python/src/metadata.rs
Outdated
| }; | ||
|
|
||
| format!( | ||
| "Metadata(content_length={}, content_md5={}, content_type={}, etag={}, mode={}, last_modified={})", |
There was a problem hiding this comment.
I believe mode is the most important metadata, and content_md5 can be removed since we don't encourage users to rely on it.
So the final list will be:
- mode
- content_length
- content_type
- last_modified
- etag
Signed-off-by: yihong0618 <[email protected]>
|
This is not a breaking change, right? I'm guessing we don't need the |
yihong0618
changed the title
OK, and FYI, it kind of small change the behaviour of print... |
I'm not familiar with python. Does python ecosystem treat such changes as breaking changes? |
if user depend the repr maybe break them, but I think its fine since we do not have repr before |
Xuanwo
changed the title
github-actions
bot
added
the
releases-note/feat
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
Thank you @yihong0618 for this work!
Xuanwo
removed
the
releases-note/refactor
3 participants
… better print and debug
Which issue does this PR close?
Closes #5768