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

Hide singleton support #307

Merged
merged 12 commits into from
Mar 13, 2020
Merged

Conversation

dinomite
Copy link
Member

@dinomite dinomite commented Mar 8, 2020

Merge after #306

As discussed in issue #281, hide Kotlin singleton object deserialization behind a feature flag, enableExperimentalSingletonSupport.

@cowtowncoder
Copy link
Member

I would actually suggest slightly different approach: use of an Enum, with descriptive names (and Javadocs matching). This because I think there are 3 options, as per earlier discussions:

  1. Normal deserialize with no special handling (pre-2.10) -- possibly the default choice
  2. Deserialize then canonicalize (i.e. deser but drop, use singleton): was default in 2.10
  3. "Dummy" deserialize: skip content, return singleton. New choice that is probably better than (2) in that no alternate instances are created at all (but could hide issues wrt JSON structure?)

The main benefit, I think is just that there may be other approaches to add in future. Enum may also be used with annotations just in case there needs to be per-class override (granted for that may want programmatic access or something).

@dinomite
Copy link
Member Author

Great idea, I'll get that updated shortly. Minor side benefit: one less boolean.

@dinomite
Copy link
Member Author

Updated with an enum, let me know what you think. If this looks good, I'd like to merge this and leave the enhancements (dummy deserialize, those described in #281 etc.) to a separate PR.

@dinomite dinomite marked this pull request as ready for review March 12, 2020 00:25
@cowtowncoder
Copy link
Member

Great, looks good. Apologies for delays; I will try to follow up more closely as this is the number 1 priority for me to get included before doing first (and maybe only) 2.11 pre-release candidate.

@cowtowncoder cowtowncoder merged commit 1f5c9fe into FasterXML:2.11 Mar 13, 2020
@dinomite
Copy link
Member Author

👍 An RC for this sounds like a good idea to me.

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