General improvements from my fork

[devkabiir]

I have made some general improvements in my fork. If you like those improvements, I'll send you a PR :).

Edit: my improvements include

1. Using const for things that are constants instead of final

2. Add @override annotation as a convention
   Example:

   + @override
     Future<Response<Account>> createAccount({Account data}) async {...}

3. Removed type information from generated methods, Dart can automatically infer the types from the super class devkabiir/retrofit.dart@ea051356ac24d85a932bf68f80c3a5fdc37c5073
   Example:

   - Future<Response<Account>> createAccount({Account data}) async {...}
   + createAccount({data}) async {...}

4. Use forwarding/redirecting constructor instead of instance() static method
   Example:

   -   static RestClient instance([Dio dio]) => _RestClient(dio);
   +   factory RestClient([Dio dio]) = _RestClient;

5. Don't add check for baseUrl being null because it's a constant and we can know that at build-time devkabiir/retrofit.dart@f5664c7a58d12ec396bb0b83d7993ca3f48ab1da
   Example:

     _RestClient([this._dio]) {
   -    final baseUrl = 'https://httpbin.org/';
   -    if (baseUrl != null && baseUrl.isNotEmpty) {
   -      _dio.options.baseUrl = baseUrl;
   +      _dio.options.baseUrl = 'https://httpbin.org/';
   -    }
     }

6. Add ArgumentError.checkNotNull for required parameters devkabiir/retrofit.dart@6c675756e2eedf943d0263b0f940a99373a0bedb

7. Generate *.g.dart shared parts instead of *.retrofit.dart parts

8. Implement the annotated class instead of extending it
   Example:

   -    class _RestClient extends RestClient {
   +    class _RestClient implements RestClient {

9. Don't assign defaultValue if it's null for parameters devkabiir/retrofit.dart@a610e2675c84376c56f9568b9c5f1476b9995596

10. Add @Extra for passing extra options to requestOptions devkabiir/retrofit.dart@1d470c296efc46c0e500e60287ed187a3244b60a
    Example:
    myapi.dart

      @http.POST('/account/')
    + @dio.Extra({'myKey': 'myValue'})
      Future<Response<Account>> createAccount(
          {@required @http.Body() Account data});

    myapi.g.dart

      @override
      createAccount({data}) async {
        ArgumentError.checkNotNull(data, 'data');
    +   const _extra = <String, dynamic>{'myKey': 'myValue'};
        final queryParameters = <String, dynamic>{};
        final _data = data;
        return _dio.request('/account/',
            queryParameters: queryParameters,
    -       options: RequestOptions(method: 'POST', headers: {}),
    +       options: RequestOptions(method: 'POST', headers: {}, extra: _extra),
            data: _data);
      }

[trevorwang]

Pull request is really appreciated. @devkabiir

[ipcjs]

8. Implement the annotated class instead of extending it

Why use implement?

When I wanted to add normal methods to the annotated class, a compilation error occurred.😵

[devkabiir]

Extending classes is very tricky. It would require a lot of conditional code generation and a lot of testing edge cases. Because you can never really know how the user will create a new object of type RestClient at static-time . In implements version, a redirecting factory constructor is used which makes sure that the generated class is used for object instantiation. A similar problem occurs if you add final instance fields the generator doesn't know how to create a class that can instantiate that final field.

Now for your use case of adding normal instance methods, you can create extensions for your RestClient class.

// MyRestClient is the name of your custom rest client
// If you're not sure about extensions in dart, there are plenty of articles available online if you search for "extension methods in dart"
extension SpecialMethods on MyRestClient {

  void myMethod(){}

  Future<void> asyncMethod() async {}

  // Literally any kind of method, getter and setter are supported.
}

[Akash98Sky]

@devkabiir you are correct.

For that reason I added some logic to keep both implements and extends in generator depending on how programmer designes annoted class.

https://github.com/trevorwang/retrofit.dart/pull/236/files#diff-c32d94163bf53716580c4c7c7effcf0bR76
Here I created a list of only generative constructor(other than default constructor).

https://github.com/trevorwang/retrofit.dart/pull/236/files#diff-c32d94163bf53716580c4c7c7effcf0bR89
And if that list is empty just implement it as the generator does earlier.

And if not empty then extend the class, coz it seems the programmer is upto adding some instance fields or some concrete methods with body. And this gives them the power to do so.

https://github.com/trevorwang/retrofit.dart/pull/236/files#diff-c32d94163bf53716580c4c7c7effcf0bR114
Here this method generates suitable constructors to match the corresponding annoted class' generative constructors.

And for you query related to final instance field.
If annoted class has final instance fields, that class must have atleast one generative constructor to initialize the final field. And thus any other class can't implement that annoted class with generative constructor. So the generated code is already broken. In that case extending the class resolves the issue.

Here's an example how this can be used to increase the flixibility:
https://github.com/Akash98Sky/onedrive_dart/blob/master/onedrive_api/lib/src/services/onedrive_api.dart

[devkabiir]

This smells bad code hygiene to me.

In your example use case onedrive_api.dart. All of your additional methods could've been extension methods as specified in my previous example. Your only valid use case for using extends is to be able to pass credentials using annotated class constructor. Which you immediately pass to new Dio instance. The whole point of a rest api service class is being able to have deterministic behavior as well as be able to test using mocks/stubs/fakes. You don't pass a dio instance to your annotated class so it's impossible to write deterministic tests for your library.

In your PR #236 You aren't handling default values for constructor parameters as well as parameters marked with the new requried keyword or the old @requried annotation.

The generated class becomes stateful, which is bad for testing as well as bad for the concept of REST api. A REST Api service's only purpose is to perform REST compliant operations on an endpoint (some refer to it as base_url).

If someone uses your stateful REST service like this and uses that instance throughout the application. They don't know for sure if the correct credentials are being used everywhere because they don't have access to the underlying Dio instance. They can't do proper error handling and they can't intercept dio requests and responses. People who use Dio, use it for it's flexibility, otherwise they can use the built-in HTTPClient. Your package uses Dio but it makes it useless for people who use Dio, they can't properly test their app/package with 3rd party code (which is your package in this case).

final apiForUser1 = OneDriveAPI(/* credentials */);

What if they want to logout the user?
What if they want to login different user?

They have to create a new instance of OneDriveAPI and with that another Dio instance.

An alternative solution for providing credentials would be to extend Dio itself and require that instead of creating a final instance field.

// Approach one
mixin DioWithCredentials on Dio {
	AccessCredentials get credentials;
}

// Approach two
mixin DioWithAutoUpdatingCredentials on Dio {
  AccessCredentials _credentials;
  AccessCredentials get credentials => _credentials ?? throw StateError('You have to set credentials first');
  set credentials(AccessCredentials newCredentials){
  	options.headers['Authorization'] = newCredentials.authorization;
    _credentials = newCredentials;
  }
}

// Using Approach one
@RestApi(baseUrl: API_BASE_URL)
abstract class OneDriveAPI {
	factory OneDriveAPI(DioWithCredentials dio) => 	
		_OneDriveApi(dio..options.headers['Authorization']=dio.credentials.authorization);
}

// Using Approach two
@RestApi(baseUrl: API_BASE_URL)
abstract class OneDriveAPI {
	factory OneDriveAPI(DioWithAutoUpdatingCredentials dio, AccessCredentials credentials) => _OneDriveApi(dio..credentials = credentials);
}

Now it's upto the user of your API to use the provided DioWithCredentials mixin they can choose to mock it or not.

Approach two also allows your users to change the credentials dynamically anytime they want using dio.credentials = /* new cerdentials */ Without creating new instance of OneDriveAPI.

There are many more ways to solve the problem without changing the generated class to extends instead of the immutable version implements.

[Akash98Sky]

Any call to the RestApi would happen via the abstract layer of annoted class only, so it's immaterial to use required on parameters of the generated class. Moreover the generated class doesn't use any fields or concrete methods added to the annoted class, so the safety of those params or methods are the duty of the annoted class designer.

As, I was trying to build a easy to use, type safe API for Onedrive using dart, that's why I didn't keep the direct access of dio open to the user using my package. OneDriveAPI is the entry point for the api and all the calls and errors relevant to dio will be handled underneath. Though it's not complete yet.