ConfigValue.As*<type> type conversions

[anuptalwalkar]

Opening an issue for discussion.
GFM-198

As methods are currently described and implemented as methods that would return the value in the desired type, and panic when they cannot convert. For example, AsString() method on ConfigValue currently returns fmt.Sprintf("%v", value). As a result AsString() returns string representation of an object, including slice or map.

Is this correct behavior for the API? Should there be a stricter API which would error out on any type mismatch?

Alternate behavior is that we want error to be returned if there is a type mismatch rather than casting value into the desired type. We can have two options - update As* methods with much stricter type checks and errors, or provide some form of strict APIs StrictString() or just String() APIs?

Although I might be minority here. To me throwing an error seems overkill considering the caller specifically requested ConfigValue as described in the method signature. The caller shouldn't be expecting error on mismatched type and avoid getting into expected panicking code paths.

Thoughts?

[anuptalwalkar]

@uber-go/service-framework

[glibsm]

@anakryiko

[glibsm]

It's an interesting discussion, I think there are two separate points here:

1. Wether or not AsString when called on any data type should be ok
2. If As* methods should panic

Based on the quick survey of the codebase it looks like all As* already panic, apart from one very specific case of AsString:

    valueType := reflect.TypeOf(value)

    if valueType.AssignableTo(targetType) {
        return value, nil
    } else if targetType.Name() == "string" { // <--------------
        return fmt.Sprintf("%v", value), nil
    }

It looks to a specially handled case, when someone is asking for a string type. I think we should remove that else if and then the problem would go away. You wouldn't be able to get a map value accidentally out as a string and it would make our API consistent in all cases.

Then, we can talk about removing the As* methods all-together and forcing people to check the ok bool, since panic() should be reserved for extreme cases.

[anakryiko]

I believe having AsStringXXX() (which can panic) and TryAsXXX() (which never panics) is fine. It's like two ways to cast values in Go, one panics if something is not as expected, while another gives programmer flexibility to fall back to something else.

I have related question, though. Should AsInt() try to truncate float64 value to int? Or it should be treated as unexpected type of value and panic? I lean toward strictness (history shows that strictness is preferred over time, as Javascript history shows).

If you still decide to truncate float64 to int, then I'd say it's logical to say that AsFloat() should upconvert int to float64.

BTW, should AsFloat() be called AsFloat64()?

[madhuravi]

I like having two methods too. We initially had a Get and MustGet version in our repo which had a similar idea. It's nice to have the strict version so users don't have to check for things.

I prefer strictness with float64 vs int too. It might be unintentional and cause bugs if we truncate it.

[shawnburke]

I think having two versions is basically a requirement, so we're all set there. I absolutely think we should be doing some set of coercions for people, it makes code so much simpler to write. For example string -> int and int -> string are strightforward.

The relevant question I see is the behavior with string conversion as a generic path. It's very common for frameworks to do "best effort" string (toString) conversions, that's not that unusual.

If the argument here is for the case where a user actually wants a string, then they can call Value() and try the type assertion as well.

But I think we could go either way on this, but I don't know what it means to fail to convert to a string. How do we know what %v does and if that's what the user did or didn't want? In most cases, it'll be right or else the user will do something custom.

[anuptalwalkar]

Thanks for all the great inputs. To summarize, we want to keep best effort type conversion with As* methods. Overall consensus to As* API usage is to make the code simpler to write and not panic on type failures.
If there is a need for stricter Value APIs, we will reevaluate introducing Must* methods. Please feel free to open the issue or relate if it leads to a scenario for introducing strict type check methods.