-
Notifications
You must be signed in to change notification settings - Fork 412
Add JsonKey.required, plus other things #221
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
Put logic in one place to make future additions easier
json_annotation/CHANGELOG.md
Outdated
`MissingRequiredKeysException` that is thrown when `required` fields don't | ||
have corresponding keys in a source JSON map. | ||
|
||
* Added a new `$` helper function and deprecate an old one. Upgrading to the |
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.
so mysterious
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.
ack
/// `JsonSerializable.disallowUnrecognizedKeys` is `true`. | ||
/// | ||
/// Should not be used directly. | ||
@Deprecated('Code generated with the latest `json_serializable` will use ' | ||
'`\$checkKeys` instead. This function will be removed in the next majoy ' |
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.
s/majoy/major
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.
ack
} | ||
|
||
/// Helper function used in generated code when | ||
/// `JsonSerializable.disallowUnrecognizedKeys` is `true`. |
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.
Or if any fields have JsonKey.required
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.
ack
/// | ||
/// Should not be used directly. | ||
void $checkKeys(Map map, | ||
{Iterable<String> allowedKeys, List<String> requiredKeys}) { | ||
if (map == null) return; |
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.
Should this still happen if requiredKeys
is not empty?
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.
ack – although look at my fix
var invalidKeys = map.keys.where((k) => !allowedKeys.contains(k)); | ||
if (invalidKeys.isNotEmpty) { | ||
throw new UnrecognizedKeysException( | ||
new List<String>.from(invalidKeys), map, allowedKeys.toList()); |
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.
lets move this toList
up to the original assignment so its consistent
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.
ack
json_serializable/CHANGELOG.md
Outdated
* Added support for `JsonKey.required`. | ||
* When `true`, generated code throws a `MissingRequiredKeysException` if | ||
the key does not exist in the JSON map used to populate the annotated field. | ||
* Will be captured captured and wrapped in a `CheckedFromJsonException` if |
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.
nit: double captured
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.
ack
@@ -128,7 +128,7 @@ class _GeneratorHelper { | |||
<String, FieldElement>{}, (map, field) { | |||
if (!field.isPublic) { | |||
unavailableReasons[field.name] = 'It is assigned to a private field.'; | |||
} else if (jsonKeyFor(field).ignore == true) { | |||
} else if (jsonKeyFor(field).ignore) { |
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.
hmm so its important now that anybody using the newest json_serializable changes there json_annotation dependency as well to have the minimum bound of 0.2.8... but there is no way to really communicate that.
I am not sure what the right solution is here?
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.
ack
Will allow for other checks to be added
No description provided.