amplify-codegen icon indicating copy to clipboard operation
amplify-codegen copied to clipboard

Datastore codegen created models don't respect Dart linting rules

Open cto-leaps opened this issue 3 years ago • 6 comments

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
[!] 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)```

</details>

cto-leaps avatar Mar 21 '21 21:03 cto-leaps

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

Wish list

  • Generated model code should at least pass all pedantic linter rules and not rely on implicit or dynamic casts. Other more strict options welcome (e.g. lint rules).
  • Generated filenames should match Effective Dart Style.

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

kjones avatar Mar 22 '21 02:03 kjones

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/*"

Jordan-Nelson avatar Sep 29 '21 22:09 Jordan-Nelson

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

dpilch avatar Oct 05 '21 18:10 dpilch

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 avatar Nov 17 '21 18:11 Jordan-Nelson

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 avatar Nov 08 '22 17:11 Jordan-Nelson

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.

Jordan-Nelson avatar Mar 10 '23 20:03 Jordan-Nelson