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

SimpleContainer not using the default constructor. #372

Closed
adupontxpertdoc opened this issue Sep 28, 2016 · 3 comments
Closed

SimpleContainer not using the default constructor. #372

adupontxpertdoc opened this issue Sep 28, 2016 · 3 comments
Milestone

Comments

@adupontxpertdoc
Copy link

Hi,

SimpleContainer is calling the wrong constructor should call AddinOptionsModel() instead of AddinOptionsModel(string filename). Example of code:

public class AddinOptionsModel
{
    // Expected constructor to be called.
    public AddinOptionsModel()
        : this("Default")
    {
        // ...
    }

    // Actual constructor called with fileName = null crashing the application. 
    public AddinOptionsModel(string fileName)

    {
        // ...
    }

}

// In Bootstrap Configuration
this._container.Singleton<AddinOptionsModel, AddinOptionsModel>();

// In our code
this.AddinOptionsModel = IoC.Get();

Regards

@nigel-sampson
Copy link
Contributor

Right now SimpleContainer is as the name says pretty simple. It simple matches on the public constructor with the most parameters irregardless of whether it can fulfill those dependencies.

It wouldn't be difficult to change this. I'd look at the SelectEligibleConstructor method of SimpleContainer. Happy to take a PR on this.

@nigel-sampson nigel-sampson added this to the v3.1.0 milestone Oct 2, 2016
@jongleur1983
Copy link

jongleur1983 commented Oct 2, 2016

What's the best strategy to implement here?
If I understand the current SimpleContainer implementation correct, it's currently using the constructor providing the most number of parameters.

static ConstructorInfo SelectEligibleConstructor(Type type) { return (from c in type.GetConstructors().Where(c => c.IsPublic) orderby c.GetParameters().Length descending select c).FirstOrDefault(); }

This is the best selection when we don't know anything about the parameters provided as it promises the largest flexibility to use all arguments.

But as far as I understand the SimpleContainer we know all parameters that can be reliably resolved.
While trying to implement that solution, I'm therefore happy to get feedback on this proposed strategy:

When selecting the right constructor (SeletEligibleConstructor), I propose to choose the public one with least unregistered parameters (types).

This is in fact a breaking change to what happens now, but if this issue should be implemented at all, I don't see how that should be possible without breaking changes.

@nigel-sampson
Copy link
Contributor

I agree with the proposal around which constructor to use, also agree that it's a breaking change so will shift it to 4.0.0.

@nigel-sampson nigel-sampson modified the milestones: v4.0.0, v3.1.0 Oct 2, 2016
jongleur1983 added a commit to jongleur1983/Caliburn.Micro that referenced this issue Oct 2, 2016
…Container

Before always the constructor with the most arguments was chosen.
Now I chose the constructor with the best ratio between arguments that can be resolved and those that can't.
jongleur1983 added a commit to jongleur1983/Caliburn.Micro that referenced this issue Oct 2, 2016
nigel-sampson pushed a commit that referenced this issue Jun 28, 2019
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

3 participants