fix: handle values with multiple types (including null) [SCH-1643] #57
Conversation
This file contains 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
colinking
changed the title
colinking
commented
Mar 2, 2019
| @@ -217,10 +223,18 @@ + (SEGExampleEvent *)initWithBlock:(SEGExampleEventBuilderBlock)block | |||
| @throw [NSException exceptionWithName:@"Missing Required Property" reason:@"SEGExampleEvent is missing a required property: requiredInt" userInfo:NULL]; | |||
| } | |||
|
|
|||
| if (builder.requiredNullableString == NULL) { | |||
| @throw [NSException exceptionWithName:@"Missing Required Property" reason:@"SEGExampleEvent is missing a required property: requiredNullableString" userInfo:NULL]; | |||
| } | |||
There was a problem hiding this comment.
Note: nullable types are now supported (as in, the client will compile), but this runtime validation is (clearly) not the desired behavior. However, this will change when we add in a full JSON Schema library to replace these required property checks, so I'm not going to bother with changing this behavior.
f2prateek
approved these changes
Mar 5, 2019
colinking
added a commit
that referenced
this pull request
Mar 18, 2020
Our Android example application stores Typewriter generated files at `com.segment.analytics`, which concealed a bug in #57 where `com.segment.analytics.Properties` (from `analytics-android`) was not being imported. This PR: - Moves the generated Typewriter client in the Android example app to `com.segment.generated`. - Adds `com.segment.analytics.Properties` as an import in generated files. - Switches `List<Object>` for `List<Properties`, since `Objects` would otherwise just be `.toString`-ed when the event is fired - Fixes serialization for `List<T>` by casting to `List<Properties>`
# for free
to join this conversation on GitHub.
Already have an account?
# 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 extends our test suite to cover JSON Schemas with properties that specify union types (
type: ["string", "number"]), including union types with anulltype (type: ["string", "null"]).Specifically, this resolves an issue with the Android client not generating enums (and instead crashing during
typewriter gen-android).While testing the example Android app against Turo's inferred spec, I also noticed that there were collisions between property key names, because similar properties (like
optional anyandoptional_any) would map to the same key identifier (OPTIONAL_ANY_KEY). This was fixed by inlining these values.I also identified an issue with event name collisions, specifically if you have two event names that differ by delimiter characters (f.e.
check_in_eventandcheckin_event). They both map to the same filename (causing one of the files to be overwritten). In this example case,CheckInEvent.javaandCheckinEvent.java(note: file systems are case-insensitive). Java will end up complaining because the class name (eitherCheckInEventorCheckinEvent) differs from the filename.This can be solved by an upstream PR to QuickType (see our conversation in Slack), but I'm not going to block this PR on that. The fix would be to make the
Namercase-insensitive so that it uses a suffix in both the class and file names to differentiate between these events.The simplest solution for affected users is to remove all but one of the colliding events. Usually these events appear due to inferring on bad data, so it usually won't be a problem.
I've added a warning that notifies users when their Tracking Plan contains events that collide in Android.
I also made a few refactoring changes:
inputDatais duplicated across all QuickType-based clients, so I moved that out toutils/rules.tsPropertiesSerializablebecause it wasn't necessary