Carapacik / swagger_parser

Dart package that takes an OpenApi definition file and generates REST clients based on retrofit and data classes for your project.
https://pub.dev/packages/swagger_parser
MIT License
94 stars 43 forks source link

1.7.0 Flag 'mark_files_as_generated' and refactoring #90

Closed StarProxima closed 11 months ago

StarProxima commented 11 months ago

It might be worth adding a pretty comment for the generated files to indicate that they are generated, indicate the source of generation and disable linter for them.

Example:

//  ____ _ _ _ ____ ____ ____ ____ ____     ___  ____ ____ ____ ____ ____
//  [__  | | | |__| | __ | __ |___ |__/     |__] |__| |__/ [__  |___ |__/
//  ___] |_|_| |  | |__] |__] |___ |  \ ___ |    |  | |  \ ___] |___ |  \                            
//
// GENERATED CODE - DO NOT MODIFY BY HAND
// ignore_for_file: type=lint

import 'package:dio/dio.dart';

import 'client/client_client.dart';

abstract class IRestClient {
  ClientClient get client;
}

/// Petstore `v1.0`
/// 
/// A sample API that uses a petstore as an example to demonstrate features in the OpenAPI 3.0 specification
class RestClient implements IRestClient {
  RestClient({
    required Dio dio,
    required String baseUrl,
  })  : _dio = dio,
        _baseUrl = baseUrl;

  final Dio _dio;
  final String _baseUrl;

  ClientClient? _client;

  @override
  ClientClient get client => _client ??= ClientClient(_dio, baseUrl: _baseUrl);
}
Carapacik commented 11 months ago

I'm not sure that this is necessary, because these files are not created by build_runner

StarProxima commented 11 months ago

I'm not sure that this is necessary, because these files are not created by build_runner

Yes, but does it make a difference to the end user? These generated files are not meant to be changed, as they can be re-generated the next time swagger_parser is run and the changes can be lost.

I think that in a team, a developer who has not personally configured the generation through swagger_parser might get the false impression that these files are handwritten.

Wouldn't it be better to specify that these files are generated so that developers, especially not very experienced ones, don't have problems with losing changes? Also, such a comment disables linter, which fixes problems with sorting imports, for example.

If it is still necessary to make changes in the code, this comment will make it clear that it is better to remove these files from this directory.

It seems to me that such a comment provides more clarity.

Sancene commented 11 months ago

These files are not meant to be generated by all project members really but rather are just a quick setup for existing swagger schema so files should be committed and be perceived as handwritten. I do not believe that this change is necessary especially the linter ignore part. We can make a flag that is defaulted to true for generated code comment but i dont see a reason why.

StarProxima commented 11 months ago

These files are not meant to be generated by all project members really but rather are just a quick setup for existing swagger schema so files should be committed and be perceived as handwritten. I do not believe that this change is necessary especially the linter ignore part. We can make a flag that is defaulted to true for generated code comment but i dont see a reason why.

There are cases where the backend is under development, often new methods are added and existing methods are modified, names and models are changed.

When this happens, it is crucial not to make manual changes to code that will change quite often over time, so a comment like this would be extremely helpful.

Ignoring the linker in such a case also has advantages, because the analyzer will not take into account the problems that may arise with conflicting linter rules (always_use_package_imports, directives_ordering, etc) in the code that you are not responsible for, because it is generated.

That's why I think it's worth adding an optional flag to the configuration as you suggested.

If you confirm this solution, then I will try to finish the pull request today.

StarProxima commented 11 months ago

@Carapacik @Sancene could you please take a look?

I've added an optional flag to generate a comment in the files.

Also small improvements on the code in FillController and Generator so that the default config parameters are only set in one place, fields are now immutable.

Also cleaned up the code in YamlConfig, reduced unnecessary checks, fields are now immutable.

I also tested all the configuration parameters, apparently, the behavior has not changed. Automated tests were also successful.

If you have any questions or comments, please let me know.

StarProxima commented 11 months ago

@Carapacik I fixed some of the comments and left a comment on the rest. If you still have any doubts, please let me know.