Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shopper Insights - Merchant analytics #869
Shopper Insights - Merchant analytics #869
Changes from all commits
2a9e084
1929260
20ab070
c5c025e
55104a6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be my lack of Android knowledge but can we just do
import com.braintreepayments.api.ShopperInsightsAnalytics
or do we need to import each event individually?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other modules we have been fully qualifying the events when used (ex: on line 61
braintreeClient.sendAnalyticsEvent(ShopperInsightsAnalytics.PAYPAL_PRESENTED)
) that being said, either way is fine with meThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we could use a wildcard import here
import com.braintreepayments.api.ShopperInsightsAnalytics.*
, but Google is opinionated about import statements. https://developer.android.com/kotlin/style-guide#import_statementsI think there are pros and cons to both ways, but Android Studio is pretty good at formatting imports and auto imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for sending over that doc!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the cons of using wildcard imports might no longer apply to kotlin, as it gives us a way to use import alias as shown below, but the tools and practices are slower to catch up to the capabilities of the newer languages, so we just go with whatever the tools suggest :D
But yeah, no reason not to use wild card imports in this current scenario.
Example of import alias.
See: https://kotlinlang.org/docs/packages.html#imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are interested, here's how things could go wrong using wildcard imports.
https://www.javadude.com/posts/20040522-import-on-demand-is-evil/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a way to turn off autogen of
import *
in Android Studio in preferences, I forget exactly where.