Add constructor functions for each variant of Val

[ickshonpe]

Objective

Add constructor functions for each variant of Val
Fixes #15937

Solution

Add constructor functions for each variant of Val.

Doc comments feel a bit clumsy, not sure how to rephrase them, maybe doesn't matter. Perhaps it's not necessary to repeat the enum documentation either, not sure.

[TheBlckbird]

I like this, but what about doing something like mentioned in #15943 (review):

pub trait AsValNum {
    fn as_val_num(self) -> f32;
}

impl AsValNum for i32 {
    fn as_val_num(self) -> f32 {
        self as f32
    }
}

impl AsValNum for f32 {
    fn as_val_num(self) -> f32 {
        self
    }
}

pub fn px(n: AsValNum) -> Val {
    Val::Px(n.as_val_num())
}

I think, that'd be nicer than doing 10. in my opinion and might allow for some future changes without breaking old code.

[ickshonpe]

I like this, but what about doing something like mentioned in #15943 (review):

pub trait AsValNum {
    fn as_val_num(self) -> f32;
}

impl AsValNum for i32 {
    fn as_val_num(self) -> f32 {
        self as f32
    }
}

impl AsValNum for f32 {
    fn as_val_num(self) -> f32 {
        self
    }
}

pub fn px(n: AsValNum) -> Val {
    Val::Px(n.as_val_num())
}

I think, that'd be nicer than doing 10. in my opinion and might allow for some future changes without breaking old code.

I think so too but afaik the type inference will break with bsn!. Also, the changes with this PR are super noncontroversial. It's an easy decision to merge them without discussion, that's also why I split off the From<f32> impl into a seperate PR.
We can always look into improving the implementation in a follow up, once this is merged.

[TheBlckbird]

I think so too but afaik the type inference will break with bsn!. Also, the changes with this PR are super noncontroversial. It's an easy decision to merge them without discussion, that's also why I split off the From<f32> impl into a seperate PR. We can always look into improving the implementation in a follow up, once this is merged.

ok, that makes sense

[alice-i-cecile]

Adding to the 0.17 milestone and assigning @cart to review. I just want to be sure that this is in line with the final plan: we've gone back and forth a ton here but I don't want to leave users hanging longer if we're ready to decide.

[cart]

The alternative to this is adding use Val::* to the prelude, which feels less "special" to me, ties the docs directly to Val, keeps our code size down, and aligns with Rust std patterns (ex: Some, None, Ok, Err, etc).

The only problem is right now bsn! breaks on this case (as it assumes it can do parent_type.value.0 to set Some(x)). This is obviously a problem! The solution is to use if let to destructure all types. I believe this will also solve one of the outstanding autocomplete issues (and it will remove the need for the touch_type hack).

[cart]

I'm going to strongly advocate that we do use Val::* instead.

[TheBlckbird]

I think it just comes down to whether we wanna allow more types besides just f32 or not #20518 (comment).
But I think a decision needs to be made and you're the one to do it. I personally like the idea of being able to omit the decimal seperator, especially because this PR is about code readability, but it is ultimately up to you.

I just think, it would be nice to ship this in 0.17 (although it isn't important enough to be blocking, as @alice-i-cecile said on discord)

[cart]

Arg yeah that does make this comfier. And currently type inference actually doesn't break in bsn! right now, as function arguments don't have implicit into().

[cart]

Arguably less rusty, and it means we're encouraging people to throw a bunch of "int to float" casts everywhere. But I'll admit to hitting the "oops please make it a float" friction on the regular.

[cart]

It does also mean we'll likely need to special case these functions somehow in BSN asset files (if we want to support them there). But I'm content making that a problem for future us.

Consider this approval for function helpers with type conversion.

[TheBlckbird]

Sorry to bother you one more time, but now I'm a bit confused: what do you actually mean by this? That we might add automatic type conversion in a later PR or that it will be done by BSN?

But thanks for finally merging this, this will already make things way easier than before!

[alice-i-cecile]

It will be done by BSN. There's automatic .into() calls via the macro now.

[cart]

@alice-i-cecile thats not what I meant in this case (see my message above about function parameters not having implicit into). Instead, we should add a trait as mentioned elsewhere.