sksamuel / hoplite

A boilerplate-free Kotlin config library for loading configuration files as data classes
Apache License 2.0
923 stars 74 forks source link

feat: EnumDecoder: support case-insensitive decoding #391

Closed monosoul closed 1 year ago

monosoul commented 1 year ago

Adds support for case insensitive decoding of enums to EnumDecoder

sksamuel commented 1 year ago

Nice idea. Can we make this an option in the config builder, so we don't enable it by default for everyone.

monosoul commented 1 year ago

@sksamuel my idea was to make ignoring case disabled by default (to preserve the current behavior). And then when someone needs to ignore case when decoding enums, they can do this:

ConfigLoaderBuilder.default()
    .addDecoder(EnumDecoder(ignoreCase = true))
    .build()
sksamuel commented 1 year ago

Ok that makes sense, but will the extra decoder override the normal decoder ?

monosoul commented 1 year ago

@sksamuel ah, right, I guess I see your point. This change (71a88da4354d5edfb02725f8d5e29d23b8baa26b) will make the extra decoder be prioritized over the default one. A bit hacky tho :sweat_smile:

sksamuel commented 1 year ago

What about adding an option to ConfigLoaderBuilder, which is then passed into the DecoderContext. Then it becomes available inside the standard enum decoder, which can switch on it.

It's much more visible that way than people adding in their own decoder.

monosoul commented 1 year ago

@sksamuel what if the default decoder is not registered? If I create an empty config builder like this ConfigLoaderBuilder.empty(), then this option will have no effect which could be misleading I think. Unless using this option will implicitly register a new decoder if it's not there yet.

sksamuel commented 1 year ago

With or without the new option, if you elect to register an enum decoder (by using empty) then you'd not get enums. I think its reasonable to assume the option would do nothing in that case. I think using empty is rare.

monosoul commented 1 year ago

@sksamuel Feels wrong to me to have a configuration option specific to a single decoder that might not even be registered, while the rest of configuration seems to be decoder-agnostic (if I don't miss anything). What do you say if I add an argument to default() and addDefaultDecoders() methods of ConfigLoaderBuilder like this:

diff --git a/hoplite-core/src/main/kotlin/com/sksamuel/hoplite/ConfigLoaderBuilder.kt b/hoplite-core/src/main/kotlin/com/sksamuel/hoplite/ConfigLoaderBuilder.kt
--- a/hoplite-core/src/main/kotlin/com/sksamuel/hoplite/ConfigLoaderBuilder.kt  (revision 71a88da4354d5edfb02725f8d5e29d23b8baa26b)
+++ b/hoplite-core/src/main/kotlin/com/sksamuel/hoplite/ConfigLoaderBuilder.kt  (date 1695033562717)
@@ -75,7 +75,7 @@
      * If you wish to avoid adding defaults, for example to avoid certain decoders or sources, then
      * use [empty] to obtain an empty ConfigLoaderBuilder and call the various addDefault methods manually.
      */
-    fun default(): ConfigLoaderBuilder {
+    fun default(ignoreEnumCase: Boolean = false): ConfigLoaderBuilder {
       return empty()
         .addDefaultDecoders()
         .addDefaultPreprocessors()
@@ -160,7 +160,7 @@
    * Adds all default [Decoder]s into this builder.
    * The decoders are located via java's [ServiceLocator] framework.
    */
-  fun addDefaultDecoders() = addDecoders(ServiceLoader.load(Decoder::class.java, classLoader))
+  fun addDefaultDecoders(ignoreEnumCase: Boolean = false) = addDecoders(ServiceLoader.load(Decoder::class.java, classLoader))

   /**
    * Adds the default [Resolver]s to the end of the resolvers list.

I understand it's a bit more invasive than just having that option in the decoder context, but at least from the API perspective there would be no way to have a configuration that doesn't make sense. :slightly_smiling_face:

sksamuel commented 1 year ago

I don't like that because we're special casing the enum handling inside the builder. Which means as more options are added, the builder ends up having all the options the config builder does. The option doesn't have to be limited to a single decoder. There might be other enum cases, or perhaps the option might be extended to support Objects in lowercase too.

monosoul commented 1 year ago

The option doesn't have to be limited to a single decoder. There might be other enum cases, or perhaps the option might be extended to support Objects in lowercase too.

Ah, okay, that makes sense. What should I call it then? Just ignoreCase?

sksamuel commented 1 year ago

resolveTypesCaseInsensitive ?

monosoul commented 1 year ago

@sksamuel f09cdf736dab8cab536413923bf13ae23bca077f

sksamuel commented 1 year ago

Looks great.

sksamuel commented 1 year ago

I'll release 2.8.0 RC2

alourie commented 6 months ago

@sksamuel We're using this library in production and require case-insensitive enum parsing. Would appreciate this release to made official. Would that be possible to release as 2.8.0?

sksamuel commented 6 months ago

It's on my radar for this weekend actually.

sksamuel commented 5 months ago

Didn't get to it yet, but will this week is my aim.

monosoul commented 4 months ago

@sksamuel sorry for nagging, but is there any chance we'll see it in the coming week or so? :sweat_smile: