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

Add KotlinPropertyNameAsImplicitName option #686

Merged
merged 5 commits into from
Jul 28, 2023

Conversation

k163377
Copy link
Contributor

@k163377 k163377 commented Jul 15, 2023

Added option to use property names in Kotlin as getter names.
This resolves #630.

Comment on lines 47 to 61
/**
* By enabling this feature, the property name on Kotlin will be used as the getter name.
*
* By default, the name based on the getter name on the JVM is used as the accessor name.
* This name may be different from the parameter/field name, in which case serialization results
* may be incorrect or annotations may malfunction.
* See [jackson-module-kotlin#630] for details.
*
* By enabling this feature, such malfunctions will not occur.
*
* On the other hand, enabling this option increases the amount of reflection processing,
* which may result in performance degradation for both serialization and deserialization.
* In addition, the adjustment of behavior using get:JvmName is disabled.
*/
UseKotlinPropertyNameForGetter(enabledByDefault = false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cowtowncoder
I would like to get a review as I am not sure about the option name and description.

Copy link
Member

Choose a reason for hiding this comment

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

One quick question: is "property" first-class concept in Kotlin? If not, maybe it'd be "field", but I assume it is.

But I guess I am not sure what "use property name for getter" actually means... I mean, getter in Jackson specifically means method use to access value of a logical property to serialize. It has name specified by class file, and does not change.
Name of logical property does change tho. So maybe naming here is bit confusing, at least to me.

Copy link
Contributor Author

@k163377 k163377 Jul 17, 2023

Choose a reason for hiding this comment

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

is "property" first-class concept in Kotlin?

I think we can call it a first-class concept in this regard.
On Kotlin, fields and accessors are defined for properties.
https://kotlinlang.org/docs/properties.html

So maybe naming here is bit confusing, at least to me.

I don't know if this is a good name, but at least this is how I would describe the internal behavior directly.

If the emphasis is on naming on Jackson, would it be UseKotrinPropertyNameForGetterImplicitName?
However, it doesn't seem like a good name since we may not use the findImplicitPropertyName function forever.
Also, the name seems too long compared to the traditional KotlinFeature.

Copy link
Member

Choose a reason for hiding this comment

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

I just did not understand what "for getter" means. But yes, if it changes "implicit [property] name" in context of serialization, I can see how "....ForGetterImplicitName" could work.

Does this not affect deserialization? Otherwise just something like "UseKotlinNameAsImplicitName" would make more sense to me.

Copy link
Contributor Author

@k163377 k163377 Jul 21, 2023

Choose a reason for hiding this comment

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

I just did not understand what "for getter" means.

I wanted to express that it does not affect parameters, fields, or setters, but only getters.
Currently we are already using names on Kotlin for parameter names, but that is not affected by this option.

Does this not affect deserialization?

If there are annotations for deserialization on the getter, it will affect the behavior, but basically it should only affect serialization.

Copy link
Member

Choose a reason for hiding this comment

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

Right. I think referring to serialization makes more sense because getter really is physical thing (Method) used to access a property for purpose of (de)serialization. So you can't really change name of getter, but you change name of property for (de)serialization. Although, yes, obtained from getter method.

Still, ultimately this is your decision: I just offer my suggestions. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed for correction.
aa7217f

@k163377 k163377 changed the title Add UseKotlinPropertyNameForGetter option Add KotlinPropertyNameAsImplicitName option Jul 28, 2023
@k163377 k163377 merged commit b523bb2 into FasterXML:2.16 Jul 28, 2023
12 checks passed
k163377 added a commit to k163377/jackson-module-kotlin that referenced this pull request Jul 28, 2023
@k163377 k163377 deleted the feat/kotlin-name branch July 28, 2023 15:25
k163377 added a commit that referenced this pull request Jul 28, 2023
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.

About the problem that property names in Jackson and definitions in Kotlin are sometimes different.
2 participants