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

[joeldomke]

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

Related issues: #24, #891

[google-cla]

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.

[joeldomke]

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

[kevmoo]

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

[joeldomke]

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]

Since the JsonKey annotation can't be added to setters, we still need a getter.

[joeldomke]

I guess it would be possible to also support fields that only have setters? @kevmoo would you consider this PR, if I implemented this, or are there other issues with this PR or just limited time/resources from your side?

[joeldomke]

I added a fifth option, currently called legacy. Fields that use this option should behave as fields that previously used ignore: false. Currently this is the default option.

[kevmoo]

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?

[joeldomke]

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.

[kevmoo]

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