Analytics - Update Methods , Clean Code #21
Conversation
...d/src/main/kotlin/com/amazonaws/amplify/amplify_analytics_pinpoint/AmplifyAnalyticsBridge.kt
Show resolved
Hide resolved
| AmplifyAnalyticsPinpointPlugin analyticsPlugin = new AmplifyAnalyticsPinpointPlugin(); | ||
| amplifyInstance.addPlugin(analyticsPlugins: [analyticsPlugin]); | ||
|
|
||
| var isConfigured = await amplifyInstance.configure(amplifyconfig); |
There was a problem hiding this comment.
Not adding the plugins anymore? Also - while you don't need to assign the result of configure to a var, since it's void, you could still catch an error.
There was a problem hiding this comment.
Thanks yeah should be adding those plugins :) Sure, I'll catch it then
fjnoyp
force-pushed
the
amplify_analytics_combineMethod
branch
from
33fdce1 to
1e96e03
Compare
# Conflicts: # packages/amplify_analytics_pinpoint/android/src/main/kotlin/com/amazonaws/amplify/amplify_analytics_pinpoint/AmplifyAnalyticsBridge.kt
Include suggestions from Michael for Readme file.
fjnoyp
force-pushed
the
amplify_analytics_combineMethod
branch
from
1e96e03 to
4060176
Compare
|
Merge requests resolved, also added Mlab's readme file changes |
| Amplify.Analytics.flushEvents(); | ||
| result.success(true); | ||
| Amplify.Analytics.flushEvents() | ||
| result.success(true) |
There was a problem hiding this comment.
Do you have some lint tooling available that can enforce this as part of the build/PR process?
|
|
||
| for (name in propertyNames) { | ||
| Amplify.Analytics.unregisterGlobalProperties(name); | ||
| if(propertyNames.size == 0){ |
| try { | ||
| Amplify.Analytics.unregisterGlobalProperties() | ||
| result.success(true); | ||
| else{ |
| _amplifyConfigured = true; | ||
| }); | ||
| } catch (e) { | ||
| print(e); |
There was a problem hiding this comment.
If the configuration fails, that probably means you won't be able to use Analytics, right? So, log-n-gobble may not be the best strategy here. Should you not have the exception bubble back up to the user, so they know not to proceed? (Or, can make the decision themselves, as to whether or not they want to, at least. Right now, you make that choice for them.)
| public static func unregisterAllGlobalProperties(result: @escaping FlutterResult){ | ||
| Amplify.Analytics.unregisterGlobalProperties() | ||
|
|
||
| if(propertyNames.count == 0){ |
There was a problem hiding this comment.
Meh, yea -- these are driving me nuts though. I don't know any language where whitespace isn't used around control flow keywords. The Dart style guide does not contain any examples of this. https://dart.dev/guides/language/effective-dart/style
| Future<void> registerGlobalProperties( | ||
| {@required AnalyticsProperties globalProperties}) async { |
There was a problem hiding this comment.
Join back to one line? Why break? What is the line wrap strategy that is being used in this code base? Has your team come to an agreement?
Running this opinionated and langauge-idiomatic formatting tool would probably be a good step in your build process.
https://dart.dev/guides/language/effective-dart/style#do-format-your-code-using-dartfmt
https://github.com/dart-lang/dart_style/wiki/Formatting-Rules
Quick PR to update methods to combine unregisterGlobalProperties with unregisterAllGlobalProperties
Minor other changes related to style and spacing
Removed optional ; in kotlin to be consistent
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.