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

Injected constructor arguments #14

Open
znakeeye opened this issue Aug 26, 2022 · 4 comments
Open

Injected constructor arguments #14

znakeeye opened this issue Aug 26, 2022 · 4 comments

Comments

@znakeeye
Copy link

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Author

znakeeye commented Sep 1, 2022

Yes, I will use that approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants