Conversation
Codecov Report
@@ Coverage Diff @@
## master #93 +/- ##
==========================================
+ Coverage 77.28% 77.32% +0.03%
==========================================
Files 81 81
Lines 3782 3792 +10
==========================================
+ Hits 2923 2932 +9
- Misses 859 860 +1 |
julianrex
reviewed
Jan 3, 2019
| return carplayEvent; | ||
| } | ||
|
|
||
| + (instancetype)eventWithDateString:(NSString *)dateString name:(NSString *)name attributes:(NSDictionary *)attributes { |
Contributor
There was a problem hiding this comment.
Just a couple of questions here (which I think we've talked about before briefly):
- Why is this a class method rather than an
MMEEventinitializer? Is this for future support for differentMMEEventsubclasses? So does this mean-[MMEEvent init]is essentially "private"? If so, I wonder if it's worth having a specific initializer (in addition) and marking the defaultinitasNS_UNAVAILABLE. Thoughts? - Should this method take an
NSDaterather than a date string? Perhaps a date transformer could be supplied (somewhere).
Contributor
Author
There was a problem hiding this comment.
This PR is somewhat of a stopgap measure until we can completely remove the whitelisting that is currently in place. The doc describes some of those plans and how we might accomplish them.
- I'm not sure how the decision came about to make these class methods or what the future plans were for it. I don't see any harm in a user of the library using an initializer vs any of these class methods to construct an event however. Is there something I'm missing here?
- I agree that this could be provided in another way and potentially better way (especially once we implement date/time correction {insert Doctor Who meme}) but I think that should be done a little further down the road. For now the date string is supplied by the
eventsManagerviaMMENSDateWrapperand all that needs to be called by the client/host isenqueueEventWithName:attributes:.
Contributor
There was a problem hiding this comment.
Is there something I'm missing here?
No - more curious about future plans 😄
[...] but I think that should be done a little further down the road
Thanks for clarifying 👍
julianrex
approved these changes
Jan 3, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR will add a generic event initializer to allow for faster iterations of new events. The main purpose of bringing this out now is our performance push and the need for many new events to be added and sent quickly.
Caution should be used when using this initializer. Such as enabling debug mode on
eventsManagerand ensuring your events make it to the server. Failure to do so could result in a loss of telemetry data.