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

JSON: Fix mutable classDiscriminatorMode in config, and mark experimental in builder #2680

Merged
merged 2 commits into from
May 28, 2024

Conversation

BenWoodworth
Copy link
Contributor

@BenWoodworth BenWoodworth commented May 17, 2024

I can drop either of these commits if they're wrong, but this PR makes classDiscriminatorMode a val in the configuration, and then marks it as experimental in the builder to match the config.

I can also deprecate the config setter instead if needed, though it doesn't seem like it's necessary. In public GitHub repos it's only set in the builder, and uncommonly enough that I'd think it's unnecessary

@sandwwraith tagging you since you implemented the feature. let me know if this looks right :)

It's experimental in the `JsonConfiguration`, so this changes the builder to match.
@pdvrieze
Copy link
Contributor

While I agree that in JsonConfiguration the property should be a val, rather than a var, and this property is experimental in that location, it would still be good to do a proper deprecation of the setter.

As to marking the property experimental in the builder that is an incompatible API change to stable code. I don't think that is correct. I suspect that it was accidentally forgotten to remove the experimental annotation in the configuration (which now makes it much easier to remove the accidental declaration as var).

@BenWoodworth
Copy link
Contributor Author

That sounds fair to me! I'll update the PR when I get a chance

And for the experimental annotation, I assumed it was accidentally left out from the builder since it was missing/mismatched from the start. I agree about it feeling wrong though, code written with a stable API then becoming experimental, so I don't know the right way forward

@BenWoodworth
Copy link
Contributor Author

I amended the last commit to deprecate the classDiscriminatorMode setter instead of removing it. It'll be reported as a warning, but I could change it to an error if it doesn't need to be as gradual.

I think I only need clarification about the builder property being marked as experimental or not, and what the intent was there. :)

@sandwwraith
Copy link
Member

Yes, you're right — since this is a new feature, it should have been added as experimental to the builder, but it somehow slipped both mine and reviewer's attention :(. So we need to add @ExperimentalSerializationApi now and, yes, eventually remove the setter. I think we can deprecate it directly with Level.ERROR now. Can you please add this deprecation level to the PR?

@BenWoodworth
Copy link
Contributor Author

Sure thing! I just amended that commit, so now it's marked as an error

@sandwwraith sandwwraith merged commit c487e78 into Kotlin:dev May 28, 2024
2 of 3 checks passed
@sandwwraith
Copy link
Member

Thank you!

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