make both Percent and UDouble readonly (thus immutable). #14
Conversation
add comparison and equality operators for both. add implicit conversions to Percent from int and double. fix UDouble.EqualsWithin effectively comparing itself to itself.
| public static implicit operator Percent(int i) => new Percent(i); | ||
| public static implicit operator Percent(double d) => new Percent(d); |
There was a problem hiding this comment.
I was shying away from implicits due to the fact that percent can easily throw exceptions on construction, as it has to be within bounds 0-1. Having implicits makes it much easier to accidentally trigger that without realizing the conversions were even being invoked.
Having to specifically call the ctor/factories gives it a bit of "weight" so they're not as sideswiped.
I think I'd prefer not having these and keeping the more conscious conversion
There was a problem hiding this comment.
The reason i felt these would be useful is in Synthesis, you can't use new versions if the code references any of the "old" percents. (see the OWL Leveled Lists Additions we were chatting about).
We could make these Explicit type casts, and then, possibly? put something into the Synthesis UI that implicitly does the type casts? i don't know of a way to do an implicit type conversion between two third-party controls, but perhaps something fancy can be worked out? wouldn't want to keep more plugins on old versions needlessly, right?
Thoughts?
There was a problem hiding this comment.
For sure. The main reason I'm not too worried about that is that it's fairly normal and expected to have these kinds of breaks within Synthesis' ecosystem. Mutagen has 1000s of record definitions, and they can change either because something was wrong, bethesda itself changed something, or we wanted to make something easier but different. So similar breaks will occur, even if we avoid it this one time for Percent. Not worth making the overall API worse forever in an attempt to avoid.
This pattern of controlled backwards incompatibility is why Synthesis has the versioning mechanisms to be able to fall back to older versions (via Match) and everything else help keep things workable until someone comes along to do the minor API tweaks.
There was a problem hiding this comment.
Fair enough, did you want the implicit conversions from percent/udouble to stay in as well? At least one of those I didn't add, you did. :)
There was a problem hiding this comment.
just thought, the other option would be to auto clamp it to 0/1 instead of throwing range errors. the math operators can throw those errors too.
There was a problem hiding this comment.
did you want the implicit conversions from percent/udouble to stay in as well?
Haha yeah, I think those less scary to keep, as there's no hidden exception potential
other option would be to auto clamp it to 0/1 instead of throwing range errors
I'm wary of that route a bit, as it has high potential for very subtle bugs. I can imagine a user thinking Percent is represented 0-100, for example, passing in 55 thinking it's 55% and instead getting 100%. No compile/runtime error, just a subtle under the radar bug that they have to notice in passing. Id rather fail fast and make the user consciously call FactoryPutInRange if that was the behavior they wanted.
| @@ -108,7 +114,7 @@ public static bool TryParse(string str, out UDouble doub) | |||
|
|
|||
| public bool EqualsWithin(UDouble rhs, double within = 0.000000001d) | |||
| { | |||
| return rhs.Value.EqualsWithin(rhs.Value, within); | |||
| return Value.EqualsWithin(rhs.Value, within); | |||
There was a problem hiding this comment.
Good catch! Sounds like i gotta bite the bullet and do some Unit tests soon 😬
| if (TryParse(str, out UDouble ud)) | ||
| return ud; | ||
| return default; |
There was a problem hiding this comment.
Yeah, weird setup I had before. I almost wonder if it should be throwing. We have the TryParse alternate, the Parse should throw if it's a failure... or be renamed to ParseOrDefault or something more clear?
There was a problem hiding this comment.
Parse usually does throw an exception on failure, TryParse true/false. not sure if there is a naming convention for ParseOrDefault (which sounds perfectly understandable to me)
add comparison and equality operators for both.
add implicit conversions to Percent from int and double. fix UDouble.EqualsWithin effectively comparing itself to itself.