Feat(typing)/allow non string trait types #43
Conversation
…ow-non-string-trait-types # Conflicts: # FlagsmithClient/src/main/java/com/flagsmith/entities/Trait.kt
FlagsmithClient/src/test/java/com/flagsmith/FeatureFlagCachingTests.kt
Outdated
Show resolved
Hide resolved
…rify the original behaviour
| val identifier: String? = null, | ||
| @SerializedName(value = "trait_key") val key: String, | ||
| @SerializedName(value = "trait_value") val value: String | ||
| ) | ||
| @SerializedName(value = "trait_value") val traitValue: Any |
There was a problem hiding this comment.
I think this has gone around in a circle a little and now I'm not really sure why the attribute name actually has to change?
There was a problem hiding this comment.
Hi @matthewelwell so because we can define our own getters and constructors the old API for the users should now be exactly the same as it was. They should just get a deprecation warning that they should move to e.g. .stringValue instead.
There was a problem hiding this comment.
To achieve this we had to move the directly-mapped value property out of the way
There was a problem hiding this comment.
This look OK @matthewelwell
There was a problem hiding this comment.
But why did we need to do this? Could we just have done
@SerializedName(value = "trait_value") val value: Any... and we'd have been done?
I get that perhaps this is slightly tidier though so I'm happy for the answer to just be "because ugly" 😄
There was a problem hiding this comment.
Hi!
Linus here from Axis, I asked for the fix on this issue. If gazreese would change the type of the value there would be two consequences. My code would stop compiling when I increase the version of the library, and I would have to re-test the behaviour of all my feature flags, since it might change depending on how I set the trait.
In my case I don't have many flags to test, but for customers(api-users) who have been using the sdk for a longer time, the change might be quite tough to do since it might require a lot of testing.
As a user of this sdk I am opting for the deprecation. If you don't find this feedback useful you can just ignore it, I will not take offense.
There was a problem hiding this comment.
Thanks @linusclindgren !
This fixes #42
Description
Type of Change