LokiMidgard / PartialMixins

Extends C# with Mixins.
MIT License
24 stars 2 forks source link

Injected constructor arguments #14

Open znakeeye opened 2 years ago

znakeeye commented 2 years ago

Create a new console app targeting .NET 6. Then paste the code below.

As you can see, we quickly run into a problem when using Mixin classes where the constructor allows for injected dependencies. Is it possible to improve the library for this common use case?

Problem 1 Re-definition of the backing field is encountered in the sample below. I believe PartialMixin should simply choose one. Perhaps the one from the first attribute? I think this is a reasonable trade-off.

Problem 2 The constructor of the Mixin classes are generated as methods - without a return type. Here we should simply create a Foo constructor with all arguments from the constructors of the mixins. To avoid too complex rules, I guess we could limit support to one mixed-in constructor. E.g. in the sample below, we would simply generate this:

public Foo(IDependency dependency)
{
    this.dependency = dependency;
}

Sample

using Mixin;

namespace MyConsoleApp;

public interface IDependency {}
public class Dependency : IDependency {}

[Mixin(typeof(Bar))]
[Mixin(typeof(Baz))]
public partial class Foo
{
}

class Bar
{
    private readonly IDependency dependency;

    public Bar(IDependency dependency)
    {
        this.dependency = dependency;
    }
}

class Baz
{
    private readonly IDependency dependency;

    public Baz(IDependency dependency)
    {
        this.dependency = dependency;
    }
}

class Program
{
    public static void Main()
    {
        var foo = new Foo(new Dependency());
    }
}

image

LokiMidgard commented 2 years ago

This is a little more logic than the project was intended for… And I honestly will propably not find the time to implement this. It was not much more then preventing me from copy pasting a bunch of code again and again.

It would be possible to rename fileds (maybe only if they are private?). But Public propertys that may be part of an interface should then get implemented explicitly. This rabit whole seems to be deep…

The more logic it has the harder will it be for the user to understand.

But of course if you have a good idea to make it better, feel free to fork it, but I currently do not have time and motivation to work on this. Sorry.

znakeeye commented 2 years ago

Sure I can fork it, but if I add this feature (plus some code cleanup) would you accept a PR? Obviously, a PR requires some review from your end.

LokiMidgard commented 2 years ago

If I understand it, I would :)

But the behavior should probably be configurable, maybe with arguments for tha MixinAttirbute or more Attributes.

I currenty think the current simple copy should be the default behavior, and if an error occours that could be fixed with another behavior the error message should say so.

znakeeye commented 2 years ago

Yes, I will use that approach.