-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
ASConfiguration version check only when have json dict #971
ASConfiguration version check only when have json dict #971
Conversation
Source/ASConfiguration.m
Outdated
@@ -25,7 +25,7 @@ - (instancetype)initWithDictionary:(NSDictionary *)dictionary | |||
if (self = [super init]) { | |||
autotype featureStrings = ASDynamicCast(dictionary[@"experimental_features"], NSArray); | |||
autotype version = ASDynamicCast(dictionary[@"version"], NSNumber).integerValue; | |||
if (version != ASConfigurationSchemaCurrentVersion) { | |||
if (dictionary != nil && version != ASConfigurationSchemaCurrentVersion) { |
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.
Let's wrap the whole getting the version out of dictionary in that != nil
check:
if (dictionary != nil) {
autotype version = ASDynamicCast(dictionary[@"version"], NSNumber).integerValue;
if (version != ASConfigurationSchemaCurrentVersion) {
// ...
}
}
Ideally we could move parsing out the featureString also in that dictionary != nil
check but it's not guaranteed that the experimentalFeatures
property is initialized as 0 what ASExperimentalFeaturesFromArray
will return if you pass in a nil featureString
currently.
Looking into ASExperimentalFeaturesFromArray
though, the input parameter is marked as nonnull. Ideally we should never pass in nil
in the first place.
Either we adjust the nullable annotation / handling in ASExperimentalFeaturesFromArray
or change the whole initializer to something like:
- (instancetype)initWithDictionary:(NSDictionary *)dictionary
{
if (self = [super init]) {
if (dictionary) {
autotype featureStrings = ASDynamicCast(dictionary[@"experimental_features"], NSArray);
autotype version = ASDynamicCast(dictionary[@"version"], NSNumber).integerValue;
if (version != ASConfigurationSchemaCurrentVersion) {
NSLog(@"Texture warning: configuration schema is old version (%zd vs %zd)", version, ASConfigurationSchemaCurrentVersion);
}
self.experimentalFeatures = ASExperimentalFeaturesFromArray(featureStrings);
} else {
self.experimentalFeatures = 0; // We may should add something like ASExperimentalNone
}
}
return self;
}
Generated by 🚫 Danger |
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.
Looks good to me!
copyWithZone:(NSZone *)zone will pass in nil which cause logging falsely.
Originated in ConfigurationManager: _config = [[ASConfiguration textureConfiguration] copy];