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(dio): Allow ResponseDecoder and RequestEncoder to be async #2015

Merged
merged 10 commits into from
Nov 10, 2023
Merged

feat(dio): Allow ResponseDecoder and RequestEncoder to be async #2015

merged 10 commits into from
Nov 10, 2023

Conversation

Reprevise
Copy link
Contributor

@Reprevise Reprevise commented Oct 30, 2023 •
edited by kuhnroyal
Loading

New Pull Request Checklist

  • I have read the Documentation
  • I have searched for a similar pull request in the project and found none
  • I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
  • I have added the required tests to prove the fix/feature I'm adding
  • I have updated the documentation (if necessary)
  • I have run the tests without failures
  • I have updated the CHANGELOG.md in the corresponding package

Additional context and info (if any)

Gives more flexibility to the developer if they want to do some async work in the response decoder. I don't think tests are needed for this. I don't think this is a breaking change as the return type of ResponseDecoder is now FutureOr<String?>.

@Reprevise Reprevise requested a review from a team as a code owner October 30, 2023 19:10
ueman
ueman previously requested changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@ueman ueman left a comment

Choose a reason for hiding this comment

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

I'm not sure whether this should be considered a breaking change. I would love to know what the others think.

However, this should be tested. Please add a test where the ResponseDecoder returns a future and one where it doesn't return a future.

dio/lib/src/transformers/sync_transformer.dart Outdated Show resolved Hide resolved
@Reprevise Reprevise requested a review from ueman October 31, 2023 16:15
kuhnroyal
kuhnroyal approved these changes Nov 3, 2023
View reviewed changes
Copy link
Member

@kuhnroyal kuhnroyal left a comment

Choose a reason for hiding this comment

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

I think this is fine. Should not break anything.

@AlexV525
Copy link
Member

AlexV525 commented Nov 5, 2023

Could we address ResponseEncoder too? If so, #1709 should be resolved.

@Reprevise
Copy link
Contributor Author

Reprevise commented Nov 7, 2023 •
edited
Loading

Could we address ResponseEncoder too? If so, #1709 should be resolved.

What is ResponseEncoder? I don't see it anywhere.

EDIT: I assume you mean RequestEncoder.

EDIT 2: I can allow RequestEncoder to return a Future in this PR, or I can open another one. Up to you guys.

@AlexV525
Copy link
Member

AlexV525 commented Nov 7, 2023

Sorry, I mean the RequestEncoder. :)

@AlexV525
Copy link
Member

AlexV525 commented Nov 9, 2023

EDIT 2: I can allow RequestEncoder to return a Future in this PR, or I can open another one. Up to you guys.

@Reprevise Please continue with this PR if possible, thanks.

@ueman ueman dismissed their stale review November 9, 2023 06:30

Feedback was addressed

@Reprevise Reprevise changed the title feat(dio): Allow ResponseDecoder to return a Future<String?> feat(dio): Allow ResponseDecoder and RequestEncoder to be async Nov 9, 2023
@Reprevise
Copy link
Contributor Author

Reprevise commented Nov 9, 2023 •
edited
Loading

@AlexV525 RequestEncoder has been updated to return a FutureOr<List<int>>

@AlexV525
Copy link
Member

AlexV525 commented Nov 9, 2023

You seem to be using a conflicted main branch, which merges your changes unexpectedly.

@Reprevise
Copy link
Contributor Author

Should be fixed now.

AlexV525
AlexV525 approved these changes Nov 10, 2023
View reviewed changes
Copy link
Member

@AlexV525 AlexV525 left a comment

Choose a reason for hiding this comment

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

LGTM. You'll need to update the CHANGELOG before it gets merged.

@AlexV525 AlexV525 linked an issue Nov 10, 2023 that may be closed by this pull request
Combine Response Encoder/Decoder with Transformer or improve them to allow asynchronized method #1709
Closed
@Reprevise
Update CHANGELOG.md
19d6e84 
Signed-off-by: Benjamin <[email protected]>
@Reprevise
Copy link
Contributor Author

Changelog has been updated

@kuhnroyal kuhnroyal added this pull request to the merge queue Nov 10, 2023
Merged via the queue into cfug:main with commit 78f3813 Nov 10, 2023
3 checks passed
@Reprevise Reprevise deleted the feature/response-decoder-future branch November 10, 2023 23:31
@AlexV525 AlexV525 added the p: dio Targeting `dio` package label Nov 13, 2023
@AlexV525 AlexV525 added the s: feature This issue indicates a feature request label Nov 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexV525 AlexV525 AlexV525 approved these changes

@kuhnroyal kuhnroyal kuhnroyal approved these changes

@ueman ueman Awaiting requested review from ueman

Assignees
No one assigned
Labels
p: dio Targeting `dio` package s: feature This issue indicates a feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Combine Response Encoder/Decoder with Transformer or improve them to allow asynchronized method
4 participants
@Reprevise @AlexV525 @kuhnroyal @ueman