Skip to content

Conversation

@jorgehermo9
Copy link
Contributor

Relates to #5774

@jorgehermo9 jorgehermo9 marked this pull request as ready for review April 20, 2025 12:23
@jorgehermo9 jorgehermo9 requested a review from Xuanwo as a code owner April 20, 2025 12:23
@dosubot dosubot bot added size:S This PR changes 10-29 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
@jorgehermo9
Copy link
Contributor Author

Should we add Operation::Info to those kind of requests?

let req = Request::get("https://api.backblazeb2.com/b2api/v2/b2_authorize_account")

@Xuanwo
Copy link
Member

Xuanwo commented Apr 20, 2025

Should we add Operation::Info to those kind of requests?

No, let's just ignore those utils API calls.

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Apr 26, 2025
StatusCode::OK => {
let bs = resp.into_body();
let mut resp: ListFileNamesResponse =
serde_json::from_reader(bs.reader()).map_err(new_json_deserialize_error)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While doing this refactor I realized that, In my opinion, we should do response deserialization in all core.rs modules. For example, list_file_names should return a ListFileNamesResponse instead of Buffer and then deserialize it in the backend.

I think all methods in core.rs (not only this services, but others) should do something similar to what I'm doing here, since I don't really think it is the backend responsibility.

@jorgehermo9
Copy link
Contributor Author

jorgehermo9 commented Apr 26, 2025

Hi @Xuanwo I refactored what you said in #6066 (comment). What do you think about the changes?

@jorgehermo9
Copy link
Contributor Author

Do not review this yet, I did a mess with the git history and I mistakenly committed some yandex_disk service changes

@jorgehermo9
Copy link
Contributor Author

Fixed!

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 for working on this!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 27, 2025
@Xuanwo Xuanwo merged commit 1b31f5a 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:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants