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.

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

Make ResponseDecoder return nullable String type since the usage is r… #1455

Conversation

huhx
Copy link

@huhx huhx commented Apr 1, 2022

This PR make the ResponseDecoder return nullable String

In options.dart:

typedef ResponseDecoder = String Function(
    List<int> responseBytes, RequestOptions options, ResponseBody responseBody);

The usage of this function: src/transformer.dart

String? responseBody;
if (options.responseDecoder != null) {
  responseBody = options.responseDecoder!(
    responseBytes,
    options,
    response..stream = Stream.empty(),
  );
} else {
  responseBody = utf8.decode(responseBytes, allowMalformed: true);
}

As we can see the responseBody is nullable, it means ResponseDecoder function return String? may be make sense.

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.

Hi @huhx, since you've mentioned the transformer, can you add a check to return an encoded body when the response body bytes are not empty? Such as:

String? responseBody;
if (options.responseDecoder != null) {
  responseBody = options.responseDecoder!(
    responseBytes,
    options,
    response..stream = Stream.empty(),
  );
} else if (responseBytes.isNotEmpty) { // <-- Add this condition to avoid an empty string response.
  responseBody = utf8.decode(responseBytes, allowMalformed: true);
}

Also, are you willing to contribute by letting us pick the PR to another community-maintained dio? (We'll announce the new repo later once we merge PRs as much as possible.) Also, it might be better to add some tests.

@@ -153,7 +153,7 @@ class DefaultTransformer extends Transformer {
} else {
responseBody = utf8.decode(responseBytes, allowMalformed: true);
}
if (responseBody.isNotEmpty &&
if (responseBody !=null && responseBody.isNotEmpty &&
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (responseBody !=null && responseBody.isNotEmpty &&
if (responseBody != null &&
responseBody.isNotEmpty &&

Copy link
Author

Choose a reason for hiding this comment

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

sure, and I already add test if options.responseDecoder return null value.

@AlexV525
Copy link
Member

@huhx Can you make a new PR based on the develop branch? Otherwise, it'll be impossible to merge your PR alone.

@huhx
Copy link
Author

huhx commented Oct 21, 2022

@huhx Can you make a new PR based on the develop branch? Otherwise, it'll be impossible to merge your PR alone.

👌

@huhx huhx changed the base branch from develop-5.0 to develop October 21, 2022 00:20
@huhx huhx changed the base branch from develop to develop-5.0 October 21, 2022 00:21
@huhx huhx changed the base branch from develop-5.0 to develop October 21, 2022 00:35
@huhx huhx changed the base branch from develop to develop-5.0 October 21, 2022 00:37
@huhx
Copy link
Author

huhx commented Oct 21, 2022

@huhx Can you make a new PR based on the develop branch? Otherwise, it'll be impossible to merge your PR alone.

This is the new PR: #1581

@AlexV525 AlexV525 closed this Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants