Closes #22 and begins to address #23.

[SphtKr]

No description provided.

[csantero]

I like the idea of spinning out the method to determine the ID, but I think it would be better to inject a property into the constructor than to be using a singleton. Could make ModelManager implement IModelManager and inject IModelManager into the constructor. That way the user can provide their own IModelManager (say they have some weird method for resolving the ID property). This is what I'm doing with IErrorSerializer, though in that case it's not publicly accessible yet.

[SphtKr]

Okay, I partly follow...Inject a IModelManager into which constructor, and who does the injecting and how? What I like about the singleton solution is that the entry point takes care of itself--if it doesn't need custom behavior, user burden is zero.

I also had thought of a way to allow folks to substitute their own ModelManager subclass (let them either specify a Type to instantiate or an instance to use instead, and have the Instance property return that instead), but what you're suggesting would work too--I just am missing the details I think.

[csantero]

This is what I was getting at:

public class JsonApiFormatter
{
    public JsonApiFormatter() : this(new ErrorSerializer(), new ModelManager())
    {
    }

    public JsonApiFormatter(IErrorSerializer errorSerializer, IModelManager modelManager)
    {
        this.ErrorSerializer = errorSerializer;
        this.ModelManager = modelManager;
    }

    public IErrorSerializer ErrorSerializer { get; set; }
    public IModelManager ModelManager { get; set; }
...
}

I think this is a more intuitive, testable, and IoC-friendly approach. User burden is still zero if they don't want to customize the behavior. You can either use the second constructor, or use the first one and override the values after construction.

The only difference between our solutions is that if you as the user are doing something like supplying a new formatter instance every request, it's up to you to provide a single long-lived IModelManager. I suspect that this is pretty rare, and users that are doing this are probably aware of the state implications of what they are doing - especially if we mention this in the readme. But if you're still concerned about it, I suppose the default constructor could use ModelManager.Instance instead of new ModelManager().

[SphtKr]

Okay, I get it. It's essentially exactly what I did with IPluralizationService. You're right about it being more testable. No, I'm not terribly worried about a user (deliberately) doing something weird that would make the JsonApiFormatter short-lived--though I was thinking before that it might be recycled by the system, or thread-local or something...but looking at it again that really can't be the case, there must be only one in the application when you put it in WebApiConfig.Register...so using it as the cache location (indirectly) is fine.

However, on the other hand, if we make the PluralizationService belong to the IModelManager as you suggested, now the user does have to mess with ModelManager and use the non-default constructor for JsonApiFormatter--at least currently because we expect them to specify a PluralizationService. Hmm... Make the interface for setting the PluralizationService remain the same, but have the setter and getter just proxy the object to the ModelManager object? That's probably no good if we let the user set the ModelManager via a property as well--it would break the connection. Thoughts?

[csantero]

I think in this case, since the IPluralizationService is much more likely to be overridden than IErrorSerializer or IModelManager, we add a third constructor with just an IPluralizationService parameter, like so:

    public JsonApiFormatter(IPluralizationService pluralizationService)
        : this(new ErrorSerializer(), new ModelManager(pluralizationService))
    {
    }

Then we remove the public PluralizationService property. I'm generally a bigger fan of constructor injection than property injection, and this scenario is one of the reasons why. The drawback is you wind up with constructors with large number of parameters, but this can be overcome by replacing them all with a single aggregate parameter. If we get to that stage in this project it might look something like the following:

public class JsonApiFormatter
{
    private readonly IModelManager _modelManager;
    private readonly IErrorSerializer _errorSerializer;

    public JsonApiFormatter(IPluralizationService pluralizationService)
        : this(new JsonApiConfiguration(pluralizationService))
    {
    }

    public JsonApiFormatter(IJsonApiConfiguration configuration)
    {
        _modelManager = configuration.ModelManager;
        _errorSerializer = configuration.ErrorSerializer;
    }
}

public interface IJsonApiConfiguration
{
    IModelManager ModelManager { get; }
    IErrorSerializer ErrorSerializer { get; }
}

public class JsonApiConfiguration : IJsonApiConfiguration
{
    private readonly IPluralizationService _pluralizationService;

    public JsonApiConfiguration(IPluralizationService pluralizationService)
    {
        _pluralizationService = pluralizationService;
    }

    public IModelManager ModelManager
    {
        get
        {
            return new ModelManager(_pluralizationService);
        }
    }

    public IErrorSerializer ErrorSerializer
    {
        get
        {
            return new ErrorSerializer();
        }
    }
}

[SphtKr]

Yeah, I see what you're saying. Aesthetically, I'm not a fan of cephalopod constructors...but on the other hand these really should be immutable choices, so property injection is a poor fit. Function over form I suppose. I'll rework this this morning, shouldn't take long. I'll have to update all three pull requests though...still learning the git workflow, not sure if this was the best way to do this one.