Add API to purge orphaned entries from the config

[notpeelz]

Add API to purge orphaned entries from the config.

Currently there's no way to get rid of orphaned entries from ConfigFile.OrphanedEntries, other than through reflection.

It would be helpful for implementing automatic config upgrades (e.g renaming a config section)

[ghorsington]

Moved this to discussions since this requires a bit more API planning.

Right now the following API was proposed:

bool TryGetOrphanedEntry(ConfigDefinition entry, out string val);
bool TryGetOrphanedEntry(string section, string key, out string val);
ReadOnlyCollection OrphanedEntires { get; }
bool TryRemoveOrphanedEntry(ConfigDefinition entry);
bool TryRemoveOrphanedEntry(string section, string key);
void RemoveAllOrphans();

Courtesy of @Windows10CE (feel free to provide reasoning for the API choices).

I think this might be a good start. Do feel free to give other suggestions.

[Therzok]

Having a class encapsulating the Orphaned entries that is accessible from ConfigFile - I think it can even be a ConfigFile itself, would help encapsulate the logic.

Especially since someone might still want to work with Orphaned entries (i.e. reacting to them being set).

[Therzok]

If the goal of OrphanedEntries is to be removed eventually, a migration mechanism might be needed.

[Windows10CE]

Waiting on additional discussion about inclusions for this API in #210

[Windows10CE]

Was mentioned in a PR review, but I'll also put it here.
Peelz proposed a method that would pop a value from the OrphanedEntries dict, both giving you the ID and removing the orphan at the same time since that seems to be the use case for most of this functionality anyway.
Something like:

bool TryPopOrphanedConfig(ConfigDefinition config, out string var);
bool TryPopOrphanedConfig(string section, string key, out string var);

[Windows10CE]

They also suggested making this functionality another overload for TryRemoveOrphanedEntry, I'm willing to do either so input about preference on this is welcome.

[notpeelz]

An example of how this could be used:

// Attempt to migrate *configDefinition* to *newConfigDefinition*
if (!this.Config.TryRemoveOrphanedEntry<bool>(configDefinition, out var oldValue)) return;
if (this.Config.TryGetEntry<SomeEnum>(newConfigDefinition, out var entry))
{
    entry.Value = oldValue ? SomeEnum.Foo : SomeEnum.Bar;
}

It would also be convenient if the methods were generic instead of returning string. Deserialization of the values could be done on demand (with caching).

[notpeelz]

I think the naming used for ConfigEntryBase is misleading. The Base suffix seems to imply it can be extended, yet ConfigFile doesn't provide any means of doing that due to its interfaces (i.e. IDictionary<ConfigDefinition, ConfigEntryBase>, etc.)

MSDN docs also advise against naming abstract classes with the "Base" suffix: https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/base-classes-for-implementing-abstractions

I suggest ConfigEntryBase and the implementation (ConfigEntry) be renamed to, respectively: public class ConfigEntry and internal class ConfigEntryImpl

[GreyHak]

I love the discussion. Thank you for considering this change. Here's my two cents.

I particularly like notpeelz's TryPopOrphanedConfig option. I could do everything I need to with just that one.

notpeelz's example suggests this, but ghorsington and Windows10CE's prototypes don't show it -- Please remember that values are type T, not type string.

[Therzok]

I totally agree with that, just put it in an encapsulated config entry collection called Orphaned, to not pollute intellisense and decrease cognitive load.

[Windows10CE]

Sorry, most of these suggestions are based off the current Orphaned collection's Dictionary<ConfigDefinition, string>, I intend to include deserialization as part of Get/Pop in the long term

[Windows10CE]

TryGetOrphanedEntry has been made generic and TryPopOrphanedEntry were added in the latest commits to #210

[ManlyMarco]

Well technically the Orphaned API could be used to list arbitrary user-defined config data.

This isn't something we planned to have in the config system, and I think it's out of scope. You can use a normal setting to store any arbitrary data that you want. API for orphaned entries wasn't planned either, but it makes sense to migrate settings, hence why my suggestion for the TryMigrateOldSetting method.

[notpeelz]

This isn't something we planned to have in the config system, and I think it's out of scope.

Having an API for managing orphaned entries is good but I think it could be made more flexible so that plugin developers aren't limited in how they want to manage unbound entries.

Orphaned entries are nothing more than key/value pairs that didn't match any ConfigDefinition. I don't see why they should be constrained to a more restrictive API.

[ManlyMarco]

I don't see why they should be constrained to a more restrictive API.

Because the underlying implementation might change in the future and exposing too much would make that more difficult without breaking backwards compatibility.

[notpeelz]

The config is essentially just a glorified key-value store. I don't see how something could so drastically change as to break backwards compatibility.

Considering the PR in its current state, it is more than workable for the majority of use cases.

 the underlying implementation might change in the future

Can you elaborate?

[ManlyMarco]

Config might be changed to use a json/xml library for example and no longer have the key/value pairs stored directly. At some point there was talk about making it possible to switch underlying implementations for use on different platforms and such (for example to store config online or in a zip file).

[ManlyMarco]

What do you think about an API like this?

void TryMigrateOrphanedSetting(ConfigDefinition old, ConfigDefinition new, Func(string, string) valueConverter);
ReadOnlyCollection OrphanedSettings { get; }
bool PurgeOrphanedSettingsOnSave { get; set; }

The usage would be to call TryMigrateOrphanedSetting before calling Bind. This would convert old setting to new format if the entry exists and remove the old entry.
I'm not sure about RemoveAllOrphans since it might be called too early before all Bind calls are done and cause you to lose values of these settings silently, hence why I changed it into a property that will remove orphaned entries when the file is being saved.

[ghorsington]

From discussions here I understand that the main purpose for manipulation of orphaned entries is to either

• get rid of them,
• change them to a new value, or
• rename the key.

From this, I agree that something like this would work better down the line, especially since we will need migrations when we allow to change the underlying config writer/reader.

My suggestion would be however to make TryMigrateOrphanedSetting a bit more general in the following sense:

void PurgeOrphanedEntries(Action<ConfigDefinition, string> valueConverter = null);

This would automatically iterate through all orphaned settings (which right now is a collection of KeyValuePair<ConfigDefinition, string> like defined in ConfigFile.cs) and allow you to migrate the value to a new entry.

If valueConverter is not given, all orphans are just purged.

Because of how orphaned entries work in general, you need to do migrations after you Bind config options:

MyPlugin()
{
    config1 = Config.Bind(...);
    newConfig = Config.Bind(...);
    ...
    
     // Migration has to be done last since all entries are considered orphaned until bound
     Config.PurgeOrphanedEntries((def, value) => {
          if (/* check key/value with def */)
              newConfig.Value = /* reparse `value` to new value. */
     });
}

[ManlyMarco]

That's cleaner, yeah. It would limit you to doing all migrating in a single spot and might mess with future Binds though. What about making it not remove settings immediately and instead either make the action return a bool that indicates success, or have it set a flag that will remove orphaned settings when saving (that way you can call this method multiple times and bind after calling it without issue).