Update default values API

[ivadyistvan]

This update changes the way how the default values defined. It moved to an extension function instead of the second argument of the delegate function.

This change was necessary to prepare the code for my other PR (#27) what makes the key argument of the delegate functions optional.

[zsmb13]

I'll comment on both PRs in one place here, given that the ultimate goal is to achieve the second one.

This is an interesting proposition - I considered adding support for auto-keyed properties before, but ultimately decided not to, mostly because it didn't seem very important to have. Looking at it again, this feature could probably be worth adding.

There are some really nice solutions here technically speaking, however, I also see some possible problems with the specific proposed API:

• DelegateWithDefault will box all default values, as it treats them as a generic T. In the current implementation, primitive default values are kept as primitives (for example, in IntDelegateWithDefault).
• We fetch the property name a lot in the delegate's code, the key parameter that the delegates work with should remain non-null. To achieve this, we'd have to add a provider for each primitive type.
• We need to expose new types from the library, such as KeyDelegate and KeyDelegateProvider. One of the main design goals of the library is to hide all delegates behind the Kotlin standard library's delegate interfaces.

What do you think of an API that instead of chaining would use a separate set of functions to achieve auto-keyed properties? E.g.

val username: String? by autoStringPref()
val username: String by autoStringPref(defaultValue = "user")

Name is subject to change there, could also be something like simpleStringPref.

[ivadyistvan]

Thanks for the review!

As an everyday user of the Krate this is always a problem, especially for new projects, to determine the coding conventions of the keys that doesn't really matters in the end. Should it be camelCase or snake_case, should we define it as a const in the companion object or just let it be inline. For me usually the inline version with the name of the propery fits the best, but everybody define them in a bit different way and after a short time the code become a mess, or every new member in the project will get the same review comment which is getting a bit boring. That's why I think that if there will be an auto-key feature that would save anybody from a lot of headache.

After I realized that the stringPref(key: String) and the stringPref(defaultValue: String) has the same signature, my first thought was to introduce a set of new functions named simple{type}Pref just like you mentioned, but I haven't really liked the idea because the purpose of my improvement was to make the usage shorter and simpler and by this you have to use a longer named function to disable a customization method. I think if we go this way then the simpler name should be belong to the simpler functionality. Unfortunately this would break the API, and if somebody upgrade the dependency of the Krate careless it causes that the used to be key parameter will be transformed to an unintentional default value.

val username: String? by stringPref()
val username: String by stringPref(defaultValue = "user")
val username: String? by customStringPref(key = "username")
val username: String by customStringPref(key = "username", defaultValue = "user")

Alternatively we can change (or create a new) extension function (withKey(key: String)) to define the key explicitly (yes, i like the chaining way better 😃).

To response your raised concerns:

• I think there is no obstacle to cease the generic way and (re)create delegates per types, in that way we can bypass the boxing problem, actually I haven't thought about this before.
• To be honest I don't really understand what do you mean on your second point, but I see that I have written key ?: property.name more than it should be, so maybe that is the problem. Could you explain this please?
• I see your point about the KeyDelegate and KeyDelegateProvider. I don't have too much experience in writing libraries, so excuse me if it's a dumb question, but is it really that important to hide the delegates behind the Kotlin standard library's delegate interfaces? In my opinion it would be easier to use Krate with chaining, and it would be easier to create an extension or plugin for it (like key, default value and validation too).

[zsmb13]

First of all, I really appreciate the thought you're putting into these improvements!

Let me respond to the open points at the end first:

• I'd like to keep the key parameter of the delegates a non-nullable String so that they don't contain the logic of falling back on the property name as the default key. To achieve this, we'd have to wrap each of them into a DelegateProvider that has access to the property name before the delegate is created, and can pass in the value of key ?: property.name to the actual delegate class.
• It's not extremely important to not expose specific types from a library in general, but it's something that was a large focus of Krate's initial API design.

Overall I see how it'd be nicer to have auto-keys for properties by default, so let's achieve that somehow. The reason why the library doesn't currently contain it is pretty much exactly the clash in the stringPref case that you've mentioned in your comment 😄

Having stringPref and customStringPref (keyedStringPref perhaps) would be nice, but it would be a very ugly breaking change, where some of the API remains available but now has new behaviour, so that's unfortunately a no-go. We'll go to a 2.0 version with these changes anyway, and will deprecate some old API, but this would be too much of a shift even then.

I think that leaves just the chaining approach after all. I'll need some time to review the details of how to achieve that exactly and how clean we can make it, but the overall direction seems pretty clear.

Can you please make sure that edits from maintainers are enabled on this PR? When I get to it, I'll push my cleanup commits on this existing PR then.

[ivadyistvan]

Thanks for the answer, it's much clearer now!

I thought that the breaking API is a no-go solution, I just mentioned it for the record. 😃

Actually I checked in the Allow edits by maintainer checkbox when I've created this and my other PR, and as I see theye're checked now as well. Or am I miss something? 🙂

[zsmb13]

Ah, I didn't know where to check whether that option is enabled or not. But I think I found it just now.

Thanks!

[zsmb13]

I started slowly going through the changes and cleaning up this PR. Something interesting I noticed is that type inference will be broken for custom types (when using Gson, Moshi, or Kotlinx) when we perform chained withDefault calls.

As in, this will still work:

var listOfValues: List<TestModel>? by moshiPref("listOfValues")

However, this won't:

var listOfValues: List<TestModel> by moshiPref("listOfValues").withDefault(emptyList())

... because the compiler can't follow the chain to deduce what the type parameter of moshiPref should be. So we're forced to provide that type explicitly there (of course, it could now be dropped from the left hand side):

var listOfValues: List<TestModel> by moshiPref<List<TestModel>>("listOfValues").withDefault(emptyList())

This is also an existing problem with the chained validate call introducing the same problem though, so I don't think it's a blocker.

[zsmb13]

Another update: avoiding boxing of the primitive values we receive as parameters of withDefault would mean adding and exposing as public API new classes and types. Decided that it's not worth pursuing, using DelegateWithDefault for all types will be fine.

[zsmb13]

I've now updated both branches and I think I'm happy with all the changes. @ivadyistvan, can you please review the current state?

[stewemetal]

Dropping my 2 cents here for @ivadyistvan 's initial thoughts (I know they were written 2 months ago 😅 ). While Going back and forth in code reviews about how you handle preference keys is really not optimal, I see another - possibly minor - hidden issue with auto-keying too. Especially if you're often onboarding new developers onto existing projects.

That is, there will be little indication for the inexperienced eyes that refactoring such a preference property's name could cause headaches on app updates because of the preference key change.

That may or may not concern you or other clients of Krate, of course. Also, from the lib's point of view, this shouldn't be a factor. 🙂

I'll go through the code and do a proper review too, just wanted to leave this here.

[ivadyistvan]

I've now updated both branches and I think I'm happy with all the changes. @ivadyistvan, can you please review the current state?

Everything looks fine for me, I like the way you updated the code. Thanks!

[zsmb13]

@ivadyistvan These changes are now live in 2.0.0 on MavenCentral. Thanks so much for the contributions! 🙌