Allow to have 'value' as a parameter identifier in annotated methods

[IcarusSosie]

Currently it is not possible to have a parameter named value in methods annotated with @GET/POST/PUT...

I'm in the complex process of setting up retrofit.dart to generate an API Client from an OpenAPI specification file, which itself is generated by api-platform, the boilerplate code of which is also generated. (by a custom built code-generator based on php-generator)

To make a long story shorter, this means that the method parameters in my RestClient have identifiers based on the column names of the underlying database. Here is an example of such a method :

  /// Updates the ExampleEndpoint resource.
  @PATCH('/api/example_endpoint/{idExampleEndpoint}')
  Future<ExampleEndpoint> exampleEndpointPatch(
    @Path() String idExampleEndpoint, {
    @Field('idClient') int? idClient,
    @Field('value') String? value,
    @Field('description') String? description,
    @Field('createdAt') String? createdAt,
    @Field('updatedAt') String? updatedAt,
    @Field('deletedAt') String? deletedAt,
    @Field('client') String? client,
  });

The lack of control I have on the identifiers causes this of course. I could rename the parameter since I have an @Field() annotation anyway, but I want to limit the chance of runtime errors and special cases in my generation code. This happens mostly in PATCH methods, but also affects some filters for GET methods. Here's the generated code for this specific method :

  @override
  Future<ExampleEndpoint> exampleEndpointPatch(
    String idExampleEndpoint, {
    int? idClient,
    String? value,
    String? description,
    String? createdAt,
    String? updatedAt,
    String? deletedAt,
    String? client,
  }) async {
    const _extra = <String, dynamic>{};
    final queryParameters = <String, dynamic>{};
    queryParameters.removeWhere((k, v) => v == null);
    final _headers = <String, dynamic>{};
    final _data = {
      'idClient': idClient,
      'value': value,
      'description': description,
      'createdAt': createdAt,
      'updatedAt': updatedAt,
      'deletedAt': deletedAt,
      'client': client,
    };
    _data.removeWhere((k, v) => v == null);
    final _result = await _dio
        .fetch<Map<String, dynamic>>(_setStreamType<ExampleEndpoint>(Options(
      method: 'PATCH',
      headers: _headers,
      extra: _extra,
    )
            .compose(
              _dio.options,
              '/api/example_endpoint/${idExampleEndpoint}',
              queryParameters: queryParameters,
              data: _data,
            )
            .copyWith(
                baseUrl: _combineBaseUrls(
              _dio.options.baseUrl,
              baseUrl,
            ))));
    final value = ExampleEndpoint.fromJson(_result.data!);
    return value;
  }

Which results in the value parameter clashing with the final value declaration.

I'm not yet super familiar with retrofit.dart, so this might not be ideal, but changing the value of _valueVar from 'value' to '_value' in generator/lib/src/generator.dart seems to solve the issue for me, and would allow to use value as an identifier for method parameters in the package user's RestClient class.

It also seems to me like the current value of _valueVar, 'value', is hard-coded in some places, and should be using _valueVar or $_valueVar to lessen the risk of bugs in the generated code in case of edits. Again, I'm not quite familiar with the code, so I'd appreciate a look-over make sure the edits I've made are indeed correct.

Additionally, the local queryParameter variable in the generated code suffers from this as well. I haven't modified it in this branch yet, but I could if deemed necessary.

[trevorwang]

Thanks for the contribution.

Pls fix the test error. You can run the following commands in your local environment.

      - name: Generate code
        run: cd example && dart pub get && dart pub run build_runner build --delete-conflicting-outputs
      - name: Analyze packages
        run: PKGS="example retrofit generator" ./tool/travis.sh dartanalyzer

[IcarusSosie]

Hi !

Thanks for the instructions on running the tests, and sorry it took me a while to get back to it. I've fixed the missing _valueVar replacement, and the analyzer does not return errors anymore, only infos.

[trevorwang]

HI @IcarusSosie Thanks for the update.

The test code must be updated accordingly.

[dickermoshe]

@IcarusSosie Will you have time to update the tests? I'll fork, update tests and create a new PR if you don't have the time. It's breaking swagger_parser

[IcarusSosie]

Hi !

Sorry for taking so long to get back to this, life got in the way these last few months. I've edited the tests to match the new expected variable name _value . Tests are passing on my machine.

I've fixed a few spots where I'd wrongly assumed _valueVar should be used instead of "value", namely the calls to r.peek("value"). One of theses edits was causing some tests to fail, but not the others. I'm wondering if maybe this is a blind-spot in the tests, for which it would be nice to have extra test cases, but I'm not familiar enough with the generator to know if this is required. @trevorwang I would appreciate you input on this if you have the time.

Again, I'm sorry I left this issue hanging for so long :open_mouth: