chore: Lints

[dnys1]

Issue #, if available:

Description of changes:

• Re-enable lint checks in amplify_api and amplify_analytics_pinpoint
• Migrate amplify_core and amplify_flutter to follow amplify_lints rules
• Minor changes in amplify_auth and amplify_datastore to enable type safety in Hub.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[codecov-commenter]

Codecov Report

Merging #918 (cc49f79) into release-candidate (827171d) will decrease coverage by 0.18%.
The diff coverage is 42.80%.

@@                   Coverage Diff                   @@
##             release-candidate     #918      +/-   ##
=======================================================
- Coverage                66.33%   66.15%   -0.19%     
  Complexity                   1        1              
=======================================================
  Files                      298      300       +2     
  Lines                    10481    10512      +31     
  Branches                   445      445              
=======================================================
+ Hits                      6953     6954       +1     
- Misses                    3372     3402      +30     
  Partials                   156      156              

Flag	Coverage Δ
android-unit-tests	62.38% <40.00%> (-0.15%)	⬇️
flutter-unit-tests	59.78% <53.21%> (-0.03%)	⬇️
ios-unit-tests	70.62% <12.50%> (-0.30%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ytics_pinpoint/lib/amplify_analytics_pinpoint.dart	72.00% <ø> (ø)
...ify_api/ios/Classes/Auth/FlutterAuthProvider.swift	0.00% <ø> (ø)
...s/amplify_api/ios/Classes/FlutterApiResponse.swift	0.00% <ø> (ø)
...pi/ios/Classes/FlutterSerializedRestResponse.swift	0.00% <0.00%> (ø)
...fy_api/ios/Classes/FlutterURLSessionDelegate.swift	0.00% <0.00%> (ø)
...es/amplify_api/ios/Classes/OperationsManager.swift	51.72% <ø> (ø)
packages/amplify_api/lib/amplify_api.dart	59.37% <ø> (ø)
packages/amplify_api/lib/src/auth_token.dart	100.00% <ø> (+57.14%)	⬆️
...pi/lib/src/graphql/graphql_subscription_event.dart	88.88% <ø> (ø)
.../src/graphql/graphql_subscription_transformer.dart	95.45% <ø> (ø)
... and 51 more

[ragingsquirrel3]

Thanks for linting! I left some questions/comments, a few general things:

• auth/datastore stuff in here. Can it be avoided or at least called out in description? I get that some of the changes are related to core types file, but there seems to be some other stuff here, as well.
• amplify_core has some files/classes moved around that looked like more than linting to me, at first read, but likely I'm missing something. If that can't be avoided here, I will have to have a closer look there.
• Removing uuid dep from API interface if moving it here.

[ragingsquirrel3]

What's the reason for copying + reading copies rather than reading centralized version?

[dnys1]

Not much of a reason in the examples, since they will only ever be run from the repo. But in general, it must be copied to each package because they are shipped independently in pub. This issue is why we had to do the 0.2.4 release 😅 because it was throwing errors that the file wasn't there.

[ragingsquirrel3]

ok thanks for explanation

[Jordan-Nelson]

Left some questions, but LGTM.

[Jordan-Nelson]

Out of scope for this PR, but thoughts on adding the lint rule prefer_relative_imports?

It would require this import to be updated to the following, since this is within the same lib.

import 'graphql/graphql_subscription_event.dart';

I see we are getting avoid_relative_lib_imports via package:lints/core.

Those two rules sounds like they might be contradictory, but they are actually complimentary.

avoid_relative_lib_imports enforces that relative imports are not used across lib boundaries, and prefer_relative_imports enforces that only relative imports are used within the same lib.

[Jordan-Nelson]

The reason I would want to add it is pretty much exactly what it says in the rule description.

When mixing relative and absolute imports it’s possible to create confusion where the same member gets imported in two different ways. One way to avoid that is to ensure you consistently use relative imports for files within the lib/ directory.

I spent quite a bit of time the other week trying to figure out why event is ModelSyncEvent was evaluating to false, when event was in fact a ModelSyncEvent. It turned out that in one location a relative import was used and in another a package import was used 😅

[dnys1]

Hmmm, that sounds like a bug, if they reference the same type. Very odd, though - I wouldn't have expected that. I'm a bit torn on this one, since I've run into the following code, which I think is kind of ugly.

import '../../../../../file1.dart';
import '../../../../folder/another_file.dart';

Whereas I think absolute imports give more context to the file being imported. IMO the imports can also more easily be fixed if a file is moved and not automatically corrected.

import 'package:my_pkg/src/file1.dart';
import 'package:my_pkg/src/folder/another_file.dart';

[dnys1]

That could totally be personal preference, though. And if there is some added utility, I'm willing to adjust my preferences haha.

[Jordan-Nelson]

I would be totally fine with all of them being package imports as well. Personally I have used relative imports within the same package and named/package imports across packages, but I think thats a norm I adopted from TS.

As long as they are all package imports or all relative imports, I don't really have a preference.

Right now we allow both within a package, and Dart doesn't recognize Foo imported from package:my_pkg/file1.dart and .file1.dart as the same type.

[Jordan-Nelson]

Question: I strongly prefer the updated format here 💯 . Just confirming though, this isn't actually enforced in anyway, correct?

[dnys1]

Not yet- will be adding a lint rule at some point to enforce it: https://dart-lang.github.io/linter/lints/require_trailing_commas.html

[Jordan-Nelson]

This is great. Didn't know this existed. I have wanted the formatting package to do for a while (there is an open issue for it). I didn't think about using a lint rule.

[Jordan-Nelson]

Out of scope for this PR, but can/should we ignore certain rules for all test directories?

[dnys1]

Don't remember the context for this one. In general, no, I believe our test code should conform to the same lint rules, personally.

[ragingsquirrel3]

Thanks for updates/replies. Cheers 🚢