Skip to content

Conversation

@kevmoo
Copy link
Collaborator

@kevmoo kevmoo commented Oct 29, 2017

No description provided.

@kevmoo kevmoo requested review from jakemac53 and natebosch October 29, 2017 00:10
@kevmoo
Copy link
Collaborator Author

kevmoo commented Oct 29, 2017

I want to do 2 more flavors of changes on top of the nullable classes – so I think it justifies the complexity.

/// serialized output.
final bool includeIfNull;

/// `true` if the generator should validate all values for `null` in
Copy link
Collaborator

Choose a reason for hiding this comment

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

These comments are a bit confusing to me, maybe just change validate/verification to check/checks? Currently it sounds like its validating that things are null or something, I don't really understand it :).

Same goes for other similar comments below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup

///
/// Setting to `false` eliminates `null` verification in generated code, but
/// does not prevent `null` values from being created. Annotated classes
/// must implement their own `null` validation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

... , otherwise you may get runtime errors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did a rewrite. PTAL

Map _defaultMap() => {};

@JsonSerializable(nullable: false)
class KitchenSinkNullable extends Object
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this really a KitchenSinkNonNullable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup. PTAL

}

var includeIfNull = annotation.read('includeIfNull').boolValue;
var classIncludeIfNull = annotation.read('includeIfNull').boolValue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not a huge fan of passing this around everywhere, it might make more sense to read the JsonKey annotations upfront and fill in defaults, so you can just pass a single configuration object around? Maybe a class that wraps up a FieldElement and a JsonKey?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done...a bit... At least for the class annotation

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.

3 participants