Skip to content

Conversation

@jorgehermo9
Copy link
Contributor

@jorgehermo9 jorgehermo9 commented Apr 20, 2025

Relates to #5774. I noticed that in #5880, there was one missing request without extension. Note that #5880 included that extension in the functions under core.rs, but there was this request that was sent in backend.rs.

serde_json::from_reader(res.reader()).map_err(new_json_serialize_error)?;

let download_url = self.core.get_download_url(&file.file_id).await?;
// TODO: this request should be done in core.rs and not here
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left this TODO since I think we should refactor this and this request needs to be in core.rs. get_download_url should be renamed to something like download_file that both gets the download url & downloads it in the same function, right?

Should I refactor that in this PR? or leave the TODO?

Copy link
Contributor Author

@jorgehermo9 jorgehermo9 Apr 26, 2025

Choose a reason for hiding this comment

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

will update this PR once #6089 is merge

@jorgehermo9 jorgehermo9 marked this pull request as ready for review April 20, 2025 11:45
@jorgehermo9 jorgehermo9 requested a review from Xuanwo as a code owner April 20, 2025 11:45
@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. releases-note/feat The PR implements a new feature or has a title that begins with "feat" labels Apr 20, 2025
Copy link
Member

@erickguan erickguan left a comment

Choose a reason for hiding this comment

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

Good catch. Good plan with the context too!

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.

Thank you @jorgehermo9 for working on this and thank you @erickguan for the review.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 27, 2025
@Xuanwo Xuanwo merged commit 7c96bd2 into apache:main Apr 27, 2025
87 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer releases-note/feat The PR implements a new feature or has a title that begins with "feat" size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants