json_serializable.dart icon indicating copy to clipboard operation
json_serializable.dart copied to clipboard

Published 20 hours ago • verified publisher icon google

More control over which fields should be added to fromJson and toJson

Open joeldomke opened this issue 3 years ago • 4 comments

Adds the option to JsonKey to include fields in toJson that are not used in the factory (fromJson).

Related issues: #24, #891

joeldomke avatar Jul 15 '22 14:07 joeldomke

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Jul 15 '22 14:07 google-cla[bot]

I guess this approach you suggested could also be used. Let me know if you want me to take a look at that.

joeldomke avatar Jul 15 '22 23:07 joeldomke

I think I like the enum route. Just allows us to unify more things!

kevmoo avatar Jul 18 '22 20:07 kevmoo

Okay, I should be able to implement this. Two questions:

  1. Currently there exists the unavailableReason 'Setter-only properties are not supported'. I guess this should no longer be the case, since we offer IncludeWith.fromJson? But this would also be a breaking change for classes that have fields that only have a setter, which would then have to be annotated with @JsonKey(includeWith: IncludeWith.ignore).
  2. Same for getters. None of the 4 IncludeWith options match the old behaviour where fields without setters would be ignored in toJson if they are unused during the factory generation. This would mean that all those getters would have to be annotated with @JsonKey(includeWith: IncludeWith.ignore).

Should there be a fifth option, that matches the old behaviour, to prevent those breaking changes?

joeldomke avatar Jul 20 '22 12:07 joeldomke

So. I think I like this BUT! it's a breaking change.

We could bring it in as a non-breaking change, but we'd need to keep the existing ignore field.

Are you on Twitter ? Can you DM me so we can schedule time to chat about this?

kevmoo avatar Oct 01 '22 01:10 kevmoo

I just saw that you enabled the CI. This PR was a bit of a mess. Two different approaches, only code and no explanation. I am currently preparing a new PR that addresses the ignore related breaking change. It will have a more detailed description and a list of open questions. So you don't have to waste any time on this. I should be able to get it out next week.

joeldomke avatar Nov 11 '22 15:11 joeldomke

Can't run with this. Need to have a model that support the old and the new living together.

kevmoo avatar Nov 17 '22 23:11 kevmoo