Skip to content

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

Merged
merged 3 commits into from
Jun 6, 2018
Merged

Add JsonKey.required, plus other things #221

merged 3 commits into from
Jun 6, 2018

Conversation

kevmoo
Copy link
Collaborator

@kevmoo kevmoo commented Jun 6, 2018

No description provided.

Put logic in one place to make future additions easier
@kevmoo kevmoo requested a review from jakemac53 June 6, 2018 15:19
`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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so mysterious

Copy link
Collaborator Author

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 '
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/majoy/major

Copy link
Collaborator Author

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`.
Copy link
Collaborator

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

Copy link
Collaborator Author

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;
Copy link
Collaborator

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?

Copy link
Collaborator Author

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());
Copy link
Collaborator

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

* 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: double captured

Copy link
Collaborator Author

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) {
Copy link
Collaborator

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

@kevmoo kevmoo force-pushed the i216_require_key branch from c2bb888 to c873833 Compare June 6, 2018 16:26
@kevmoo kevmoo force-pushed the i216_require_key branch from c873833 to 923facc Compare June 6, 2018 16:33
@kevmoo kevmoo force-pushed the i216_require_key branch from c8fa6f3 to f6761f1 Compare June 6, 2018 17:03
@kevmoo kevmoo merged commit f6761f1 into master Jun 6, 2018
@kevmoo kevmoo deleted the i216_require_key branch June 6, 2018 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants