Support @AutoDefault for 'required-key-but-not-required-value' behavior

[sebastiananu]

Implemented the new annotation @AutoDefault to support, assigning a default value when the value of the property is set to 'auto'. Related issue:https://github.com/brianm/config-magic/issues/15

[brianm]

It has been a while since I have worked on this library, so it is probably something I don't remember, but.. how is @AutoDefault different from @Default ?

[brianm]

Oh, I think I get it, iff the value is "auto" it will ignore the value and use the default.

It might be cleaner to implement as another attribute on @Default, something like @Default(value="kangaroo", overrides={"auto", "défaut", "default"}) where the values in the overrides attribute are values the default value should override. There might be a better name for overrides though.

[divyekapoor]

@brianm There are 2 challenges with the suggested approach:

1. If @Default picks up a second parameter, it will force a code change on all existing code: @Default("foo") will not work anymore. The compiler will ask for @Default(value = "foo") and that's probably not desired at all.
2. The behavior of @Default is different from @AutoDefault (@Default allows an absent key; @AutoDefault does not).
3. The closest we can get to addressing your comment is to make @AutoDefault more verbose. ie. Change: @AutoDefault("value") to @AutoDefault(value = "value", for_values = {"auto", "default", "AUTO"}) - I'm not sure that's desirable, but it will work.

Couple of things that would do with your input:

1. We would prefer clarity & simplicity: @AutoDefault should override "auto" / "Auto" / "AUTO" - it's also in line with @Default. It's also follows YAGNI and is the smallest 4 chars (fewer chances of a typo).
2. If you would strongly prefer that the override string be configurable, then the proposal is: @AutoDefault(value = "foo", for_values = {"auto", "AUTO", "Auto", "default"})

Please confirm whether we should proceed with (1) or (2). I would (personally) prefer (1) but will be open to (2) if that's your preference.

Thanks for working with us!

[brianm]

@divyekapoor Re: various points

(1): If the new param for @Default has a default value (empty array), I believe it is binary backwards compatible (this is easy to test)

(2) We can get around that by another Default param, "required" which defaults to false.

I think having both Default/AutoDefault is harder to use as there are no affordances for which a user wants, just RTFM. I lean towards modifying @Default as this really is having a default... with slightly different (parameterized) behavior. I think the behavior you want is reasonable, so want to do it in the simplest (for users to get correct behavior from) way :-)

[divyekapoor]

Thanks for the feedback!

@sebastiananu just tested this - @Default("foo") works when a 2nd parameter is added with a default. The updated proposal is: @Default("foo") - works as before (implicit 2nd param's default value is "*") - key is not required. @Default(value = "foo", for_values = {"auto", "AUTO"}) - is the new addition - key is required and only if value is set to "auto" or "AUTO" will it get substituted.

Multiple @Default with different for_values will not be supported (only a single supported annotation).

Please confirm (especially the last part) and we can get going.

Cheers!

[brianm]

I would default it to the empty set, not a flag value ("*"). There is an implicit "null is also replaced by default value" when property is not present, but null as special case I think is fine. It is a pain.

Nit, I'd stick with camelCase instead of for_values as much as I prefer snake_case.

[brianm]

BTW, happy to confirm that there should not be multiple @Default annotations, that would end to horrible behavior. We should probably eagerly raise an exception (IllegalState) if it is detected!

[brianm]

Merged, am cutting release, will be 0.19

[brianm]

Hmm, something failed with the oss.s.o release, need to debug into what happened.

[sebastiananu]

Thanks, Brianm for approving the pull request. This helps a lot. Also, Just would like to double-check, have you released this version?

[brianm]

Yep, I checked and it looks like it made it to central. 0.22 should have these changes