Config Review #1

overall looks good!

i am always going to advocate for fewer nouns and i think that you can get rid of the Configurable/FromConfig duality and just have a single attribute FromConfig that is applied to fields and properties to poopulate them on load. as it stands Configurable's only role seems to be when filtering types on load and unless there's an appreciable performance difference to doing that i would suggest simplifying things.

similar thoughts about IConfiguredMember and ConfiguredDummy which don't seem to play a meaningful role.

is there a plan to support non-primitive types like Color and Vector3?

code specific comments follow.

Config.cs:15-23

        private static string SaveFilePath
        {
            get => Path.Join(UnityEngine.Application.streamingAssetsPath, "config.json");
        }

        private static string OverrideFilePath
        {
            get => Path.Join(UnityEngine.Application.streamingAssetsPath, "config.overrides.json");
        }

this is probably fine but ideally we wouldn't hard code two file names but rather have a sequence of files that we load and apply in order. we would need a way to feed that sequence to the config system at load time though so probably not worth it.

why is it called SaveFilePath?

OverrideFilePath unused.

Config/Config.cs:33-40

                // TODO probably need something more robust
                var assemblyName = assm.GetName().Name;
                if (assemblyName.Contains("UnityEngine") ||
                        assemblyName.Contains("Unity.") ||
                        assemblyName.Contains("UnityEditor") ||
                        assemblyName.Contains("Mono") ||
                        assemblyName.Contains("JetBrains") ||
                        assemblyName.Contains("System."))

StartsWith is probably going to be a bit safer than Contains but we can't do a whole lot more than this.

Config.cs:65

                var ValuesFromJson = JsonConvert.DeserializeObject<Dictionary<string, ConfiguredDummy>>(json);

why do you need ConfiguredDummy here? can't it just be object?

ConfiguredField.cs:27

                return $"{this.Assembly.GetName().Name}{ns}.{this.Type.Name}.{this.Field.Name}";

missing a . before {ns}

IConfiguredMember.cs

does anything else besides ConfiguredField implement this interface? if not, is it needed?

overall looks good! i am always going to advocate for fewer nouns and i think that you can get rid of the `Configurable`/`FromConfig` duality and just have a single attribute `FromConfig` that is applied to fields and properties to poopulate them on load. as it stands `Configurable`'s only role seems to be when filtering types on load and unless there's an appreciable performance difference to doing that i would suggest simplifying things. similar thoughts about `IConfiguredMember` and `ConfiguredDummy` which don't seem to play a meaningful role. is there a plan to support non-primitive types like `Color` and `Vector3`? code specific comments follow. --- [Config.cs:15-23](https://code.emma.coop/EMMA/unity-tools/src/branch/main/Config/Config.cs#L15-L23) ```cs private static string SaveFilePath { get => Path.Join(UnityEngine.Application.streamingAssetsPath, "config.json"); } private static string OverrideFilePath { get => Path.Join(UnityEngine.Application.streamingAssetsPath, "config.overrides.json"); } ``` this is probably fine but ideally we wouldn't hard code two file names but rather have a sequence of files that we load and apply in order. we would need a way to feed that sequence to the config system at load time though so probably not worth it. why is it called `SaveFilePath`? `OverrideFilePath` unused. [Config/Config.cs:33-40](https://code.emma.coop/EMMA/unity-tools/src/branch/main/Config/Config.cs#L33-L40) ```cs // TODO probably need something more robust var assemblyName = assm.GetName().Name; if (assemblyName.Contains("UnityEngine") || assemblyName.Contains("Unity.") || assemblyName.Contains("UnityEditor") || assemblyName.Contains("Mono") || assemblyName.Contains("JetBrains") || assemblyName.Contains("System.")) ``` `StartsWith` is probably going to be a bit safer than `Contains` but we can't do a whole lot more than this. [Config.cs:65](https://code.emma.coop/EMMA/unity-tools/src/branch/main/Config/Config.cs#L65) ```cs var ValuesFromJson = JsonConvert.DeserializeObject<Dictionary<string, ConfiguredDummy>>(json); ``` why do you need `ConfiguredDummy` here? can't it just be `object`? [ConfiguredField.cs:27](https://code.emma.coop/EMMA/unity-tools/src/branch/main/Config/InternalTypes/ConfiguredField.cs#L27) ```cs return $"{this.Assembly.GetName().Name}{ns}.{this.Type.Name}.{this.Field.Name}"; ``` missing a `.` before `{ns}` [IConfiguredMember.cs](https://code.emma.coop/EMMA/unity-tools/src/branch/main/Config/InternalTypes/IConfiguredMember.cs) does anything else besides `ConfiguredField` implement this interface? if not, is it needed?

Updates from feedback:

Condensed Configurable/FromConfig into one attribute
ConfiguredDummy was obviated by the move to XML and has been deleted
Color and Vector types are supported after moving the serialization to XML
SaveFilePath renamed to DefaultFilePath. OverrideFilePath is currently commented out. I dont have time so I made a new issue to implement it see Issue 3
Changed the assembly skip-list to use StartsWith and added more assemblies
the . is not missing it's in the ns string on purpose so we dont get .. in keys with no namespace
I eventually want to support Properties as well as Fields so I'm going to keep IConfiguredMember to be a common interface for both. see Issue 2