Add a Generic Numeric TextModel

[kaycodes13]

Summary of Changes

I know that #76 is currently slated to add support for auto-generating elements for the missing numeric value types. But I had an idea for how to reduce the number of public API methods there are that are dedicated to just creating TextModels for numbers and I wanted to propose it.

This replaces TextModels.ForIntegers and ForFloats with a method ForNumbers<T> that works for any of the built-in numeric value types. (Edited): The idea is that we keep a dictionary of numeric value types and their TryParse functions, and if the type parameter is in the dictionary, it creates a parser function using the reflected TryParse and returns a model for that. When passed a non-numeric type, it throws an ArgumentException instead.

The ConfigEntryFactory methods GenerateIntegerElement and GenerateFloatElement have similarly been replaced with a method GenerateNumberElement that catches all numeric value types.

I've tested it with the automatic menu test and haven't encountered any problems with any of the types.

I understand if this gets canned for being too clever/code smelly, lol, but I wanted to offer it anyway.

(Also, version has not been bumped due to the many open PRs at the moment)

Checklist

• No change is too small for a release, so pick one:
  • I have updated the package version following semantic versioning
  • There is another change actively WIP that will update the version (link issue or PR here)
  • This PR does not update user-facing code/config
  • I'm not sure how to set the version and would like the reviewer's help

[flibber-hk]

https://stackoverflow.com/questions/8171412/cannot-implicitly-convert-type-int-to-t/8171547#8171547

Any time you find yourself switching on a type in a generic you are almost certainly doing something wrong.

My feeling is that the advantages of this are probably not worth the code stinkiness

[kaycodes13]

The other option for doing the same thing but with slightly less switching is to use reflection.

Edit: I've now committed a version of the idea which uses reflection and a cache of delegates instead of the generic-switch.

[flibber-hk]

I think I personally prefer the existing non-reflection approach - I don’t think doing it with reflection really provides much benefit on a code cleanliness perspective given that the functions involved are pretty simple and it’s only the int and float that matter for the vast majority of cases (given that they fit in the best with the Unity ecosystem) (and obviously reflection isn’t free - even if that doesn’t really matter here). I don’t mind being overruled if more people want it like this though.

Another alternative would be writing a source generator that generates the code for each numeric type, but for this specific use case that would be a lot more work than simply doing each numeric separately (not to mention it makes the code structure a bit worse as you can’t have all the code next to each other)

[dplochcoder]

I think I personally prefer the existing non-reflection approach - I don’t think doing it with reflection really provides much benefit on a code cleanliness perspective given that the functions involved are pretty simple and it’s only the int and float that matter for the vast majority of cases (given that they fit in the best with the Unity ecosystem) (and obviously reflection isn’t free - even if that doesn’t really matter here). I don’t mind being overruled if more people want it like this though.

Another alternative would be writing a source generator that generates the code for each numeric type, but for this specific use case that would be a lot more work than simply doing each numeric separately (not to mention it makes the code structure a bit worse as you can’t have all the code next to each other)

The source-generator approach is very slightly more tempting with the ModMenuAnalyzers project established, but I also don't think it's worth it for these two use cases (TextModels and ConfigEntryFactory). It took me about ~3 minutes to copy-paste the existing Ints and Floats factories to handle all numeric types.

[flibber-hk]

The source-generator approach is very slightly more tempting with the ModMenuAnalyzers project established

FWIW, what I meant by source-generator approach was something different; basically the API surface of Silksong.ModMenu is the same, but the code is generated by a third-party package with an API like

[TextTemplate(ITER_BODY)]
[TextParameters("sbyte", "ForSignedBytes")]
[TextParameters("byte", "ForBytes")]
[TextParameters("int", "ForIntegers")]
[TextParameters("bool", "ForBooleans")]
// etc
public static partial class TextModels
{
    private const string ITER_BODY = /*lang=c#-test*/
        """
        /// <summary>
        /// An ITextModel which parses its input into a variable of type {0}.
        /// </summary>
        public static ParserTextModel<{0}> {1}() =>
            new({1}.TryParse, DefaultUnparse<{0}>);

        /// <summary>
        /// An ITextModel which parses its input into a variable of type {0} clamped between a min and max.
        /// </summary>
        public static ParserTextModel<{0}> {1}({0} min, {0} max)
        {
            var model = {1}();
            model.ConstraintFn = RangeConstraint(min, max);
            return model;
        }
        """;
}

(in other words, a more generalized version of DataDrivenConstants)

I don't know if this sort of package already exists, but if it doesn't I don't think it makes sense for it to be owned/maintained by silksong-modding.

(And obviously, as you observed, it's not worth creating such a package specifically for this use case, given that you've already done it manually and it was pretty quick to do :hornetsip: )

[kaycodes13]

FWIW the suggestion was less about the time/difficulty of copy-pasting methods and more about reducing the number of user-facing methods which do effectively the same thing - 2 methods for making numeric models rather than 22 - but if the answer is "having that many methods isn't a problem" then that also is fine.

[flibber-hk]

FWIW the suggestion was less about the time/difficulty of copy-pasting methods and more about reducing the number of user-facing methods which do effectively the same thing - 2 methods for making numeric models rather than 22 - but if the answer is "having that many methods isn't a problem" then that also is fine.

Hmm, I can’t really see a problem with having 22 methods that do basically the same thing, as long as it isn’t proving obstructive to anyone. I’m gonna close this pr now on that basis but we can reopen if you/someone are being obstructed by it