Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Classes to Support Customization via Inheritance #384

Closed
Soyvolon opened this issue Mar 26, 2024 · 1 comment · Fixed by #385
Closed

Update Classes to Support Customization via Inheritance #384

Soyvolon opened this issue Mar 26, 2024 · 1 comment · Fixed by #385
Milestone

Comments

@Soyvolon
Copy link
Contributor

Soyvolon commented Mar 26, 2024

Some situations (like #383) could be resolved by allowing a developer to create a child class of a ConverterBase (or any existing converter) and implementing their own logic or doing some post/pre processing on the value. However, the current Converter does not support creating instances of anything that is outside of the ReverseMarkdown.Converters assembly. It also doesn't support changing the loading behavior, nor modifying the loaded converters.

I propose the following:

  1. Add/Update a constructor in the Converter class to include a parameter where additional assemblies can be searched for ConverterBase classes.
    • ex: params Assembly[] additionalAssemblies
    • Could also be passed into the Config object, but may want to consider separation between loading and behavior settings.
  2. Update the constructor to stores types before activating them.
    • If a new type is a child class of an existing type, the new type replaces the existing type.
    • If a new type is a parent of an existing type, it is ignored.
  3. Make fields from private to protected.
  4. Change Config { get; } to Config { get; init; } (or protected set if init is not supported)

Additionally, some other changes can be made to make inheritance better:

  1. Mark methods in Converter as virtual.
    • ex: Convert(string html), Register(string tagName, IConverter converter) and/or Lookup(string tagName)

Mockup Constructor

I'm using the most recent C# syntax here, but it shouldn't be hard to convert to the syntax the package requires

public Converter(Config config, params Assembly[] additionalAssemblies)
{
    Config = config;

    Assembly[] assemblies = [typeof(IConverter).GetTypeInfo().Assembly, .. additionalAssemblies];

    List<Type> types = [];
    // instantiate all converters excluding the unknown tags converters
    foreach (var assembly in assemblies)
    {
        foreach (var ctype in assembly.GetTypes()
            .Where(t => t.GetTypeInfo().GetInterfaces().Contains(typeof(IConverter)) &&
            !t.GetTypeInfo().IsAbstract
            && t != typeof(PassThrough)
            && t != typeof(Drop)
            && t != typeof(ByPass)))
        {
            // Check to see if any existing types are children/equal to
            // the type to add.
            if (types.Any(e => e.IsAssignableTo(ctype)))
                // If they are, ignore the type.
                continue;

            // See if there is a type that is a parent of the
            // current type.
            Type? toRemove = types.FirstOrDefault(ctype.IsAssignableTo);
            // if there is ...
            if (toRemove is not null)
                // ... remove the parent.
                types.Remove(toRemove);

            // finally, add the type.
            types.Add(ctype);
        }
    }

    // For each type to register ...
    foreach(var ctype in types)
        // ... activate them
        Activator.CreateInstance(ctype, this);

    // register the unknown tags converters
    _passThroughTagsConverter = new PassThrough(this);
    _dropTagsConverter = new Drop(this);
    _byPassTagsConverter = new ByPass(this);
}

If you are willing to support this in the project - let me know. It shouldn't be hard for me to throw together the changes in a PR and test them. Might need some help creating a valid test for this, though (check the _converters prop after init maybe?).

Created a draft PR: #385 - I expanded on the mockup and made sure the changes worked with all the tests. Waiting on if I will need to make changes due to design decision (using Config or not, etc.).

@mysticmind
Copy link
Owner

Acknowledge seeing this, I haven't had a chance to run through this as yet fully, will do and revert.

@mysticmind mysticmind added this to the 4.6.0 milestone Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants