Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(services/azblob): Implement list support #193

Merged
merged 13 commits into from
Mar 30, 2022
Merged

feat(services/azblob): Implement list support #193

merged 13 commits into from
Mar 30, 2022

Conversation

D2Lark
Copy link
Contributor

@D2Lark D2Lark commented Mar 29, 2022

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Summary about this PR

close Xuanwo/reqsign#43

I didn't encounter this problem later, so it's better to CLOSE. I added listing function, implemented stream trait and parse XML response

Xuanwo
Xuanwo reviewed Mar 29, 2022
View reviewed changes
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM!

src/services/azblob/object_stream.rs Outdated Show resolved Hide resolved
src/services/azblob/object_stream.rs Outdated Show resolved Hide resolved
src/services/azblob/object_stream.rs Outdated Show resolved Hide resolved
src/services/azblob/object_stream.rs Outdated Show resolved Hide resolved
Xuanwo
Xuanwo reviewed Mar 29, 2022
View reviewed changes
src/services/azblob/object_stream.rs Outdated Show resolved Hide resolved
src/services/azblob/object_stream.rs Outdated Show resolved Hide resolved
src/services/azblob/object_stream.rs Outdated Show resolved Hide resolved
src/services/azblob/object_stream.rs Outdated Show resolved Hide resolved
src/services/azblob/object_stream.rs Outdated Show resolved Hide resolved
@Xuanwo Xuanwo mentioned this pull request Mar 29, 2022
Merged
@Xuanwo Xuanwo changed the title Azblob list object feat(services/azblob): Implement list support Mar 29, 2022
@Xuanwo
Copy link
Member

Xuanwo commented Mar 30, 2022 •
edited
Loading

Unresolved questions that block this PR get merged:

@D2Lark
Copy link
Contributor Author

D2Lark commented Mar 30, 2022

#193 (comment)
I have tried removing this blob var but failed. I am not so familiar with XML parsing. Is it possible to remove the Blobs?

@Xuanwo
Copy link
Member

Xuanwo commented Mar 30, 2022

I have tried removing this blob var but failed. I am not so familiar with XML parsing. Is it possible to remove the Blobs?

Oh, there is a misunderstanding here. What I mean is remove the line #[serde(rename = "Blob", default = "Vec::new")].

  • blob will be renamed to Blob via PascalCase, so we don't need to specify it again.
  • Vec already has Default implemented, so we don't need to specify it again via Vec::new.

@Xuanwo
Copy link
Member

Xuanwo commented Mar 30, 2022

Nice work!

@Xuanwo Xuanwo merged commit 0883e6e into apache:main Mar 30, 2022
@D2Lark D2Lark deleted the azblob-list-object branch March 30, 2022 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xuanwo Xuanwo Xuanwo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

azblob request will replace '/' with '%2F' in url
2 participants
@D2Lark @Xuanwo