aws-amplify / amplify-codegen

Amplify Codegen is a JavaScript toolkit library for frontend and mobile developers building Amplify applications.
Apache License 2.0
60 stars 63 forks source link

Datastore codegen created models don't respect Dart linting rules #510

Open cto-leaps opened 3 years ago

cto-leaps commented 3 years ago

Describe the bug CLI codegen models creates QueryFields that use uppercased variables.

Example: static final QueryField SESSIONID = QueryField(fieldName: "sessionID");

They get picked up by the IDE as Name non-constant identifiers using lowerCamelCase.dart(non_constant_identifier_names)

To Reproduce Steps to reproduce the behavior:

  1. Create a graphql schema
  2. Run amplify codegen models
  3. Scroll down to 'static final QueryField.'
  4. See the IDE screaming for non_constant_identifier_names errors

Expected behavior Respecting dart listing rules

Platform Amplify Flutter current supports iOS and Android. This issue is reproducible in (check all that apply): [X] Android [X] iOS

Amplify CLI version 4.45.2

Output of flutter doctor -v ```[✓] Flutter (Channel stable, 2.0.3, on macOS 11.2.3 20D91 darwin-arm, locale en-FR) [!] Android toolchain - develop for Android devices (Android SDK version 30.0.3) ! Some Android licenses not accepted. To resolve this, run: flutter doctor --android-licenses [✓] Xcode - develop for iOS and macOS [✓] Chrome - develop for the web [✓] Android Studio (version 4.1) [✓] VS Code (version 1.54.3) [✓] Connected device (3 available)```
kjones commented 3 years ago

Re-adding my wish list from aws-amplify/amplify-flutter#241.

Wish list

Turn something like this on for the amplify-flutter repo and codegen generated models and see what you get for warning/errors.

include: package:lint/analysis_options.yaml

analyzer:
  strong-mode:
    implicit-casts: false
    implicit-dynamic: false
  errors:
    missing_return: error
    missing_required_param: error
    must_be_immutable: error
    invalid_use_of_protected_member: error
    dead_code: info
    sdk_version_async_exported_from_core: ignore
    parameter_assignments: error
  exclude:
    # This next line will need to be uncommented to exclude the generated models.
    # - lib/models/*.dart

linter:
  rules:
    - unnecessary_statements
    - unnecessary_lambdas
    - avoid_classes_with_only_static_members
    - avoid_renaming_method_parameters
    - camel_case_types
    - constant_identifier_names
    - cascade_invocations
    - omit_local_variable_types
    - sort_constructors_first
    - sort_unnamed_constructors_first
    - prefer_single_quotes
    - directives_ordering
    - no_default_cases
Jordan-Nelson commented 3 years ago

Hello @cto-leaps and @kjones - Thank you for your input. This is something that we are looking into.

I think this can be broken down into two issues:

  1. The code generated models are not excluded from lint rules defined within the project. This is an issue since the generated models cannot possibly respect every possible lint rule that may be used within a project. To address this, the models probably need to be excluded from analysis in analysis_options.yaml.
  2. The code generated models are not following dart/flutter conventions. With Flutter 2.5.0+, at least some of these conventions are now being enforced in new projects with flutter_lints. If code generated models were excluded from a projects lint rules, this wouldn't be as apparent. However, the generated models should still ideally follow conventions.

At least as a temporary workaround for those facing analysis options with generated models, the following snippet can be added to analysis_options.yaml.

analyzer:
  exclude:
    - "lib/models/*"
dpilch commented 3 years ago

A new version of amplify-codegen has been released with the change from aws-amplify/amplify-codegen#248.

Jordan-Nelson commented 2 years ago

A few updates now that https://github.com/aws-amplify/amplify-codegen/pull/248 is merged.

This PR added ignores for all the rules from flutter_lints that the codegen models were not following. The goal will be to have the codegen models actually follow these rules in the future. For now, this will prevent lint issues from showing up for projects using flutter_lints. We will keep this issue open to track the work to have the models actually follow the rules.

The code generated models are not excluded from lint rules defined within the project

This was discussed a bit with the amplify-flutter team and we decided that it doesn't make sense to try to modify a projects analysis_options.yaml. Automatically updating the analysis_options.yaml file isn't very straightforward. Additionally, we ultimately want the codegen models follow flutter_lints (or a stricter rule set), so updating the files will not be needed if the project uses flutter_lints. If a project is using custom rules, the exclusion can always be added manually.

Jordan-Nelson commented 2 years ago

I am going to transfer this issue to aws-amplify/amplify-codegen as it is a more appropriate location.

The remaining work is to update the generated code to follow lint rules by flutter_lints so that the // ignore_for_file: ... can be removed from the generated code without causing lint issues for most customers.

Jordan-Nelson commented 1 year ago

As mentioned in https://github.com/aws-amplify/amplify-flutter/issues/902#issuecomment-1464364229, we may want to consider lint rules from popular linting packages (for example, very_good_analysis).

In the short term, we could ignore rules from these packages. In the long term, we can attempt to follow these rules.