search

mikhailshilkov / With.Fody

A Fody addin to extend immutable C# classes with With() methods to return a copy of an object with one property modified.
MIT License
36 stars 3 forks source link

Regression after the latest multiple constructors change #6

Closed mikhailshilkov closed 7 years ago

mikhailshilkov commented 7 years ago

@aalmada The latest constructor selection method seems to be too restrictive. I have a case when a weaver class has more properties than constructor arguments, i.e. some properties are calculated properties. Say,

public class Sum
{
    public Sum(int a, int b)
    {
        this.A = a;
        this.B = b;
    }

    public int A { get; }
    public int B { get; }
    public int Total => this.A + this.B;
}

With.Fody fails to weave this class now, which is a regression.

aalmada commented 7 years ago

What are your thoughts? What about looking for the constructor with more parameters and ignore properties that are not paired? Use an attribute to ignore properties?

mikhailshilkov commented 7 years ago

I think we should relax the condition of all properties being present in constructor parameters, and then pick the constructor with max amount of parameters which are all properties. I don't want to clutter code with any attributes.

aalmada commented 7 years ago

@mikhailshilkov Meanwhile I had started working on an attribute. I created PR #9 just so you can check how it would like. It can be ignored.

mikhailshilkov commented 7 years ago

Attributes, if we need them, should stay optional. I don't intend to use any attributes in our production code. I would suggest to wait with any attributes until we have a real case which can't be solved otherwise.

aalmada commented 7 years ago

@mikhailshilkov I implemented in #10 the fix you propose. Let me know what you think.

aalmada commented 7 years ago

@mikhailshilkov Sorry for the regression and thanks for everything