ConfigValue.As*<type> type conversions about fx HOT 7 CLOSED

@uber-go/service-framework

from fx.

@anakryiko

from fx.

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.

from fx.

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()?

from fx.

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.

from fx.

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.

from fx.

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.

from fx.