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

@Optional annotation seems excessive #19

Closed
shafirov opened this issue Oct 5, 2017 · 11 comments
Closed

@Optional annotation seems excessive #19

shafirov opened this issue Oct 5, 2017 · 11 comments

Comments

@shafirov
Copy link

shafirov commented Oct 5, 2017

For @Optional annotation default constructor parameter value is required. I guess we could consider all of parameter with default values as optional

@elizarov
Copy link
Contributor

elizarov commented Oct 5, 2017

We've decided to require it there to be explicit about optionallity (at least initially). It also helps when you start moving your properties between primary constructor and class body. The parameters in the class body must always have an initialization expression, but it may not be conceptually appropriate to tread all of them as implicitly optional. Or maybe it is appropriate. We can reconsider this, but it really needed a larger design discussion.

@apatrida
Copy link

I prefer we infer Optional and you override with Required in the cases where you don't like the default behaviour. This would also match some other implementations of data binding that have been around a bit setting expectations :-)

@pdvrieze
Copy link
Contributor

In the current state where optional values are always written anyway this may be an option, but if we have optional mean that writing may be omitted by the format(encoder/decoder) a default optionality can create surprising effects.

@rawnsley
Copy link

rawnsley commented Feb 1, 2019

I second the idea of removing Optional and replacing it with Required.

At the very least Optional should be implied for nullable types.

@Diolor
Copy link

Diolor commented Feb 5, 2019

+1 to @rawnsley 's At the very least Optional should be implied for nullable types.

If you have strong reasons to keep the existing functionality by default, another way is to make a global strategy and one to chose for himself:

@Serializable
data class Foo(
		val name: String?
)

Json(implyNullOptionals = true).parse(Foo.serializer(), "{ }") // this should print Foo(name=null)

@avently
Copy link

avently commented Feb 9, 2019

@elizarov @pdvrieze @sandwwraith
Can you take look at this issue once more?

I think that every field can be optional by default (or this option can be specified via Json constructor) and doesn't throw exceptions if it has default value.
In my situation I have client-server architecture. Configuration files can be edited by a user. He can make a mistake and then default value should be used since I don't want to give up and allow to server to die. Default values exist for every field and they are safe for every user.

What do you think now after year while this issue exist?

@pdvrieze
Copy link
Contributor

pdvrieze commented Mar 5, 2019

The way the system works is that missing values are actually verified by the (generated) (de)serializationstrategy. Any format would struggle supporting this as it would need to keep its own account of missing non-read values and implicitly read nulls. This is actually what my XML library does - it treats missing values as null (for now it doesn't yet look at optionality as that was not visible in older versions).

It may be possible to create proxy strategies that make this work with default formats. I think that would probably be the way to go.

@sandwwraith
Copy link
Member

Seems there are no cases when this feature would do any harm, so starting from 0.11.0, optional annotation is deprecated and no longer required. Any properties with default values are considered optional.

Please note that you have to use Kotlin 1.3.30 or higher.

@aayush26
Copy link

@sandwwraith What is alternative when kotlin version is < 1.3.30.
Mine is 1.3.21 and not able to find any documentation for this.

@LouisCAD
Copy link
Contributor

@aayush26 Why don't you update to Kotlin 1.3.61?

@sandwwraith
Copy link
Member

@aayush26 Specify @Optional explicitly. New library versions do not work with old Kotlin versions anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants