-
Notifications
You must be signed in to change notification settings - Fork 627
Crashlytics Add Internal Keys feature for Unity Metadata #2671
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
Conversation
samedson
commented
May 17, 2021
- Allow us to pull internal Unity Metadata without impacting customers' Custom Keys limits.
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (28bc1000) is created by Prow via merging commits: 5a0ed84 b03a676. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (c0edee6a) is created by Prow via merging commits: b5d06cd 17d2d9d. |
Getting the following failure:
|
import java.util.Map; | ||
|
||
/** Handles custom attributes internal to the SDK, eg. for custom Unity metadata. */ | ||
public class InternalKeys { |
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.
The implementations of InternalKeys
and UserMetadata
are almost identical. Did you consider making a KeyValueStore
class and making UserMetadata
extend from it? Then the internalKeys
object is an instance of KeyValueStore
. Usually I'm not too worried about DRY but I think in this case it makes more sense to use inheritance, since the two stores are so conceptually similar. Thoughts?
Macrobenchmark ReportAffected SDKsMeasurements are for head commit (17d2d9d). Diffing against base commit (b5d06cd) is working in progress.
|
@@ -461,6 +465,7 @@ private void mockEventInteractions() { | |||
when(mockEvent.getApp()).thenReturn(mockEventApp); | |||
when(mockEventApp.toBuilder()).thenReturn(mockEventAppBuilder); | |||
when(mockEventAppBuilder.setCustomAttributes(any())).thenReturn(mockEventAppBuilder); | |||
when(mockEventAppBuilder.setInternalKeys(any())).thenReturn(mockEventAppBuilder); |
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 was the necessary line to fix tests
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.
Good choice on the KeysMap!
…)" This reverts commit 79c1305.