[dio] 💡 Improve comments #1772
[dio] 💡 Improve comments #1772
Conversation
|
Thanks for the review @ueman. I was attending some offline meetings and paused the OSS work for about 2-3 weeks. Will continue soon, maybe the next Monday. |
dio/lib/src/dio.dart
Outdated
| /// dio.options.headers = {HttpHeaders.userAgentHeader: 'dio', 'common-header': 'xx'}; | ||
| /// ``` | ||
| /// 2. create and config it: | ||
| /// The abstraction class users can use directly by the unnamed constructor. |
There was a problem hiding this comment.
I don't understand what you're trying to say.
There was a problem hiding this comment.
The abstracted class for users to easily make HTTP request. ? The idea is to tell them this is a simple class they can use directly.
There was a problem hiding this comment.
I don't think we need to say that it's an abstract class. Dart & Flutter use this pattern too in various places and they never mention something like this.
There was a problem hiding this comment.
Although, I don't have a strong opinion on this. Feel free to merge anyway
There was a problem hiding this comment.
I'm going to write this to keep it simple: Dio enables you to make HTTP requests easily.
# Conflicts: # dio/lib/src/adapters/io_adapter.dart # dio/lib/src/dio_exception.dart # dio/lib/src/dio_mixin.dart # dio/lib/src/options.dart # dio/test/basic_test.dart # dio/test/cancel_token_test.dart # dio/test/stacktrace_test.dart
|
All code comments are updated (probably). |
# Conflicts: # dio/lib/src/transformers/sync_transformer.dart
Signed-off-by: Alex Li <[email protected]>
|
@ueman I'll be waiting until you've confirmed the above comments are resolved. |
| @override | ||
| Future<Response> download( | ||
| String urlPath, | ||
| dynamic savePath, { | ||
| ProgressCallback? onReceiveProgress, | ||
| Map<String, dynamic>? queryParameters, | ||
| CancelToken? cancelToken, | ||
| bool deleteOnError = true, | ||
| String lengthHeader = Headers.contentLengthHeader, | ||
| Object? data, | ||
| Options? options, | ||
| }) { | ||
| throw UnsupportedError( | ||
| 'download() is not available in the current environment.', | ||
| ); | ||
| } | ||
|
|
||
| /// Make http request with options. | ||
| /// | ||
| /// [uri] The uri. | ||
| /// [data] The request data | ||
| /// [options] The request options. |
There was a problem hiding this comment.
@AlexV525
Why is the download removed?!
I think this must be a broken change, we encountered problems in the implementation of the packages!
There was a problem hiding this comment.
It's been moved to specific adapters with more precise exceptions thrown, and it hasn't actually done anything before.
Filed PR for this: #1916
There was a problem hiding this comment.
It's been moved to specific adapters with more precise exceptions thrown, and it hasn't actually done anything before.
Filed PR for this: #1916
Yes, but at least it would have been better if the developers were informed. Anyway, thanks for solving the problem
New Pull Request Checklist
mainbranch to avoid conflicts (via merge from master or rebase)CHANGELOG.mdin the corresponding packageAdditional context and info (if any)