Skip to content
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

Allow passing instance method or conditional expressions to option ignore_serialize on JSON::Field #11804

Merged
merged 10 commits into from
Mar 14, 2022

Conversation

cyangle
Copy link
Contributor

@cyangle cyangle commented Feb 6, 2022

Closes #11803

Allow passing instance method or conditional expressions to option ignore_serialize on JSON::Field from #11803.

It seems to be trivial to implement since the compiler and stdlib are written in crystal itself.

The new specs provide example usages:

  • Use an instance method to decide whether to ignore the field
  • Dynamically emit nulls based on @{{key}}_present

@oprypin oprypin changed the title Closes #11803 Proof of concept for new annotation option ignore_serialize_if on JSON::Field New annotation option ignore_serialize_if on JSON::Field Feb 6, 2022
@cyangle cyangle marked this pull request as ready for review February 6, 2022 18:31
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@vlazar
Copy link
Contributor

vlazar commented Feb 8, 2022

@cyangle Was it not possible to make existing ignore_serialize option to do what it did before + new behavior introduced here?

@cyangle
Copy link
Contributor Author

cyangle commented Feb 8, 2022

@cyangle Was it not possible to make existing ignore_serialize option to do what it did before + new behavior introduced here?

Great idea, let me see if I could make it backward compatible or not.

…nore_deserialize

It is backwards compatible when set to `true`
@cyangle
Copy link
Contributor Author

cyangle commented Feb 8, 2022

@straight-shoota

In last commit, I implemented what @vlazar suggested.

It works and should be backward compatible.

I don't like the nested unless branches though, suggestions are welcome.

Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

At first I thought having two separate fields would be better. But I see it's probably a good idea to merge them.
I just have a few comments to simplify the implementation.

src/json/serialization.cr Outdated Show resolved Hide resolved
src/json/serialization.cr Outdated Show resolved Hide resolved
src/json/serialization.cr Outdated Show resolved Hide resolved
src/json/serialization.cr Outdated Show resolved Hide resolved
@cyangle cyangle changed the title New annotation option ignore_serialize_if on JSON::Field Allow passing instance method or conditional expressions to option ignore_serialize on JSON::Field Feb 8, 2022
src/json/serialization.cr Outdated Show resolved Hide resolved
@cyangle
Copy link
Contributor Author

cyangle commented Mar 11, 2022

@straight-shoota Could this PR be merged before the 1.4 release?

@straight-shoota
Copy link
Member

We need another approval for this. @beta-ziliani maybe?

@straight-shoota straight-shoota added this to the 1.4.0 milestone Mar 11, 2022
@straight-shoota straight-shoota merged commit e74a34b into crystal-lang:master Mar 14, 2022
@cyangle cyangle deleted the json_ignore_serialize_if branch March 14, 2022 23:10
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants