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

Creating a more versatile annoted class #236

Merged
merged 11 commits into from
Oct 6, 2020

Conversation

Akash98Sky
Copy link
Contributor

@Akash98Sky Akash98Sky commented Aug 25, 2020

Usecase:

Annoted Class

@RestApi(baseUrl: 'https://api.themoviedb.org')
abstract class MovieServiceTmp<R> {
  String _apiKey;
  final Configuration configuration;

  MovieServiceTmp._inst(this._apiKey, {this.configuration});

  MovieServiceTmp._inst2([this.configuration]);

  factory MovieServiceTmp(Dio _dio, String _apiKey,
      {String baseUrl, Configuration configuration}) = _MovieServiceTmp._inst;

 factory MovieServiceTmp.withoutKey(Dio _dio,
      {String baseUrl, Configuration configuration}) = _MovieServiceTmp._inst2;

  AssetResolver assetResolver(QualitySettings qualitySettings) =>
      AssetResolver(configuration, qualitySettings);

  Future<PagedResult<MovieBase>> search(String query, {int page}) {
    var settings = MovieSearchSettings(query: query);
    return advancedSearch(settings, page: page);
  }

  Future<PagedResult<MovieBase>> advancedSearch(MovieSearchSettings settings,
      {int page}) async {
    assert(settings.query != null && settings.query.isNotEmpty);
    var mockjson = await _search(settings, page ?? 1, _apiKey);

    var baseFactory =
        (json) => MovieBase.fromJson(json, assetResolver(settings.quality));

    return PagedResult<MovieBase>.fromJson(mockjson, baseFactory);
  }

  @GET('/3/search/movie', autoCastResponse: false)
  Future<dynamic> _search<T>(
    @Queries() MovieSearchSettings settings,
    @Query('page') int page,
    @Query('api_key') String apiKey,
  );
}

MovieServiceTmp serviceTmp =
      MovieServiceTmp(Dio(), '...');

Generated Code

class _MovieServiceTmp<R> extends MovieServiceTmp<R> {
  _MovieServiceTmp._inst(this._dio, String _apiKey,
      {this.baseUrl, Configuration configuration})
      : super._inst(_apiKey, configuration: configuration) {
    ArgumentError.checkNotNull(_dio, '_dio');
    this.baseUrl ??= 'https://api.themoviedb.org';
  }

  _MovieServiceTmp._inst2(this._dio,
      {this.baseUrl, Configuration configuration})
      : super._inst2(configuration) {
    ArgumentError.checkNotNull(_dio, '_dio');
    this.baseUrl ??= 'https://api.themoviedb.org';
  }

  final Dio _dio;

  String baseUrl;

  @override
  _search<T>(settings, page, apiKey) async {
    ArgumentError.checkNotNull(settings, 'settings');
    ArgumentError.checkNotNull(page, 'page');
    ArgumentError.checkNotNull(apiKey, 'apiKey');
    const _extra = <String, dynamic>{};
    final queryParameters = <String, dynamic>{
      r'page': page,
      r'api_key': apiKey
    };
    queryParameters.addAll(settings?.toJson() ?? <String, dynamic>{});
    final _data = <String, dynamic>{};
    final Response _result = await _dio.request('/3/search/movie',
        queryParameters: queryParameters,
        options: RequestOptions(
            method: 'GET',
            headers: <String, dynamic>{},
            extra: _extra,
            baseUrl: baseUrl),
        data: _data);
    final value = _result.data;
    return value;
  }
}

Though here type parametrized class was not required, it's just to show that it's also possible now.

Akash98Sky and others added 5 commits August 24, 2020 20:22
* the annotation class and methods can be now type parameterized

* change deprecated
  typeValue.displayName -> typeValue.getDisplayString()
* Now in annotation class extra class fields can be declared
* Can be initialized through named params of generated class constructor
* Whenever the annoted class has generative constructors
  extend that class instead of implementing it.

* Thus annoted abstract class can now have methods with
  body without the need of implementing it in super class.

* This also removes the hastle of redefining extra fields
  and initializing it, as done on prev commit 1137ea2

Signed-off-by: Akash Mondal <a98mondal@gmail.com>
Signed-off-by: Akash Mondal <a98mondal@gmail.com>
@trevorwang
Copy link
Owner

Great sample.

I would be better the response data was able to use generic data

* Annoted class can now create multiple generative constructors
  Generated class will also have multiple generative constructors
  matching each of their params

Signed-off-by: Akash Mondal <a98mondal@gmail.com>
@Akash98Sky
Copy link
Contributor Author

Akash98Sky commented Aug 29, 2020

Giving ultimate flexibility to the annoted class

Steps to write the code for annoted class (say: AnnotClass):

  1. Write an abstract class using the retrofit annotations,
  2. Design the class of your own, using class variables, methods with or without(having annotations) body, may have multiple generative constructor,
  3. Run pub run build_runner build to let the retrofit_generator generate the remaining code for you,
  4. Create one or more factory and redirect them to the constructors of generated class(_AnnotClass).
    [Generated class constructors will have the same name and params as the annoted class' generative constructors]

Your API client is ready to make request :)

[NOTE: These commits are made keeping in mind that, it doesn't break any old codes with retrofit generator]

* In some senario the check for non default constructor
  failed and the generator created named constructor with
  0 name length, so it's better to check for name length.

* refactor some pieces of code in order to supress
  some of dart analytics messages

Signed-off-by: Akash Mondal <a98mondal@gmail.com>
@stale
Copy link

stale bot commented Oct 3, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Oct 3, 2020
@stale stale bot removed the wontfix This will not be worked on label Oct 5, 2020
@trevorwang
Copy link
Owner

Sorry for the late reply.

Any idea for extending the class instead of implementing it? It broke the test on generator

@Akash98Sky
Copy link
Contributor Author

Sorry for the late reply.

Any idea for extending the class instead of implementing it? It broke the test on generator

Actually implementing the annoted(user created) class restricts the class to have only annotation methods and not any custom methods to add some extra functionality.
By just extending the class and a few more little changes mitigates this drawback, and moreover allows the programmer to have more flexibility.
And thus it adds more flexibility to the package without adding any overheads or breaking changes.

NOTE: just with this version of code, the newly generated code will be a little modified with the generated class extending the annoted class.

Here's a package I was designing a few days back with my changes on the generator:
https://github.com/Akash98Sky/onedrive_dart/blob/master/onedrive_api/lib/src/services/onedrive_api.dart

Any issue or drawbacks with my changes please let me know. I'll try to resolve as early as possible :)

* these changes are due to code refactoring, in order to
  maintain dart coding standards as mentioned below.

 - Don't access members with `this` unless avoiding shadowing.
   ref: https://dart-lang.github.io/linter/lints/unnecessary_this.html

 - Try adding a return type to the method.
   ref: https://dart-lang.github.io/linter/lints/always_declare_return_types.html

* also ignore the default constructor from annoted class constructor
  list, and thus implements the class if it's the only constructor.

Signed-off-by: Akash Mondal <a98mondal@gmail.com>
@trevorwang trevorwang merged commit ca99adb into trevorwang:master Oct 6, 2020
@trevorwang
Copy link
Owner

There are discussions about implement and extends. It will give you a context of how we don't use extends here.
#12 (comment) and #238

@devkabiir
Copy link
Contributor

Your argument for flexibility is invalid. Dio already has interceptors and transformers, which already allows for "ultimate flexibility" by adding flexibility to generated code you are only creating confusion and multiple sources of truth.

In your example you really just want easy access to configuration object, which you can do so by using any of the dozens of state management frameworks available today + extension methods on the annotated class.

@Akash98Sky
Copy link
Contributor Author

Akash98Sky commented Oct 14, 2020

  1. Extension methods were designed to add more functionality to an already defined class of dart or some other dependency packages, whose source you are not supposed to be changed explicitly. But using extension methods on a class defined on the same project are nothing but adding onto the complexity of code.
    (ref: https://dart.dev/guides/language/extension-methods#overview)

  2. On reference to your comment General improvements from my fork #12 (comment).
    Again, using a mixin on Dio just for adding extra class fields, are nothing but like a way around to cope up with the shortcoming of this package, i.e. can't add extra fields to annoted class, which again makes the code complex for just adding some fields that can just be done on the annoted class itself.

And, as I already mentioned with my version of retrofit generator, you can either opt for a completely abstract implementation of annoted class with no generative constructor and then the generated code will only implement the class.
Or if anyone wants to opt for an annoted class with generative constructor, then extend the class. And when someone is adding generative constructor on an abstract class it's obvious that they are aware of what they are doing.

Moreover, rather than being a normal functional library, this is code generation library so it should be as robust as possible. You might like to add extension methods to add extra methods and design a mixin on Dio to add extra fields. Someone else might like to keep their code simple and stupid with all relevant methods and fields on the annotation class.
This version of generator allows both of you to do so.

And, yeah if this version of generator compromises any code efficiency or performance or breaks in any circumstance, feel free to comment.
But, I'm a bit scared if you are trying to say to keep the shortcoming and go by your approach and have a way around to it. :)

@devkabiir
Copy link
Contributor

Someone else might like to keep their code simple and stupid with all relevant methods and fields on the annotation class

You're right. Impossible to fight stupidity.

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.

None yet

3 participants