Arguments class and Resolve methods refactor

[jonorossi]

This pull request addresses my code review comments from the now closed #420, and I've rewritten the docs page for arguments.

Ultimately this was to resolve #346.

@jnm2 if I can get your review on this, I'll get this merged and we can push out a beta for 5.0 with the ASP.NET Core changes.

/cc @jnm2 @mario-d-s @stakx

[stakx]

Generally looks good to me, although I didn't review extensively. Here's a little feedback after reading the long discussion in #420. Feel free to make use of, or disregard, these comments as you see fit; I don't wish to hold this PR back.

[jnm2]

I agree with @stakx's comments and I have some more minor comments.

Arguments.FromFoo(...) static factory methods don't seem to be here. I personally like those as an API better than new Arguments().InsertFoo(...) as an API, reading through the rest of the PR. Same as how I prefer ImmutableArray.CreateRange(...) to ImmutableArray<T>.Empty.AddRange(...).

I don't feel strongly since it looks like I'll just be using the Resolve(IReadOnylDictionary) extension methods anyway.

[jonorossi]

Arguments.FromFoo(...) static factory methods don't seem to be here. I personally like those as an API better than new Arguments().InsertFoo(...) as an API, reading through the rest of the PR. Same as how I prefer ImmutableArray.CreateRange(...) to ImmutableArray<T>.Empty.AddRange(...).

I don't feel strongly since it looks like I'll just be using the Resolve(IReadOnylDictionary) extension methods anyway.

There is a few different bits here I'm responding to. I know we keep making changes but I think we are nearly home with the comments.

You can't do Arguments.Empty.Add(...) here, it'll be readonly, but your example is a bit different because this isn't immutable. After now seeing everything together I don't actually see much use for the Empty field at this stage, especially that we don't actually even use it internally, and it might crop up that you get passed an Arguments instance that you can't add to in a completely different extension point in your application just because someone passed in the Empty field rather than a new Arguments(). I think we should just remove it for now.

Here is the current state in the pull request, with some comments:

Arguments()
Arguments(string name, object value)
Arguments(Type type, object value)
Arguments(IDictionary values)
// it would be good to have an IEnumerable<KeyValuePair<string, object>> constructor,
// but it'll be ambiguous with IDictionary which is more important here so you can pass
// in both named and typed parameters

void Add(object key, object value) // throws
Arguments Add(IDictionary arguments) // doesn't throw

Arguments AddNamed(string key, object value) // none of the rest throw
Arguments AddNamed(IEnumerable<KeyValuePair<string, object>> arguments)
Arguments AddNamedProperties(object instance)

Arguments AddTyped(Type key, object value)
Arguments AddTyped<TDependencyType>(TDependencyType value)
Arguments AddTyped(params object[] arguments) // this handles one or more objects
// should we have an IEnumerable overload here? Will that just be ambiguous with the object one?

I think we should do something about one Add overload throwing and the other not, I'm thinking that we just make Add(object, object) not throw.

Factory methods vs constructor arguments, I was content with the factory methods which I proposed originally but at some point along the line in the other pull request they were changed. Annoyingly the problem we've got now is that we don't have and can't have a constructor that accepts an IEnumerable<KeyValuePair<string, object>> so you have to do new Arguments().AddNamed(...) while Arguments.FromNamed(...) would look a bit nicer.

new Arguments("A", 1)
    .AddNamed("B", 2);

Arguments.FromNamed("A", 1)
    .AddNamed("B", 2);

Arguments                              // same as 2nd example just formatted differently
    .FromNamed("A", 1)
    .AddNamed("B", 2);

Arguments
    .WithNamed("A", 1)
    .WithNamed("B", 2);

I don't think we can use WithNamed for both static and instance methods because I think that implies that it doesn't modify the Arguments instance, would you assume it is a pure function? We couldn't do that and copy-on-write because there are a whole bunch of Windsor extension points that assume the collection is writable and don't pass the instance back.

If we were to change back to factory methods rather than constructors we'd remove 2 constructors and then get the benefit of being able to use all the named and typed overloads for the first set, like this:

Arguments()
Arguments(IDictionary values)

// obviously the current 3+3 AddNamed and AddTyped instance methods

static Arguments FromNamed(string key, object value)
static Arguments FromNamed(IEnumerable<KeyValuePair<string, object>> arguments)
static Arguments FromNamedProperties(object instance)

static Arguments FromTyped(Type key, object value)
static Arguments FromTyped<TDependencyType>(TDependencyType value)
static Arguments FromTyped(params object[] arguments)

@jnm2 @stakx Comments on the things I've mentioned please.

[jnm2]

You can't do Arguments.Empty.Add(...) here, it'll be readonly, but your example is a bit different because this isn't immutable.

Right. Just to clarify, it’s new Arguments().Add(...) that feels suboptimal to me. Removing Empty sounds great to me.

// should we have an IEnumerable overload here? Will that just be ambiguous with the object one?

This is a common thing to do that does not result in ambiguities, even if you pass a null literal.
Like every API, I’d be hesitant to add this before there was demand.

I think we should do something about one Add overload throwing and the other not, I'm thinking that we just make Add(object, object) not throw.

Shouldn’t they both throw? Why would it make sense for the user to pass things that Arguments doesn’t understand?

Annoyingly the problem we've got now is that we don't have and can't have a constructor that accepts an IEnumerable<KeyValuePair<string, object>> so you have to do new Arguments().AddNamed(...) while Arguments.FromNamed(...) would look a bit nicer.

I’d add a private constructor to enable Arguments.FromNamed(...) etc.

I don't think we can use WithNamed for both static and instance methods because I think that implies that it doesn't modify the Arguments instance, would you assume it is a pure function?

I’ve seen With* used in both immutable scenarios and mutable scenarios that just return this, but I agree that Add* or Replace* is clearer when dotting off an existing instance.

If we were to change back to factory methods rather than constructors we'd remove 2 constructors and then get the benefit of being able to use all the named and typed overloads for the first set, like this:

Seems good to me.

static Arguments FromNamedProperties(object instance)

Thought this one was going to be FromProperties? Not sure what a named property is.

[stakx]

Disclaimer: I don't know the Windsor code base well, so I am looking at Arguments' API in isolation.

Throwing vs. non-throwing versions of Add

TL;DR: They should throw when given invalid input.

I think we should do something about one Add overload throwing and the other not, I'm thinking that we just make Add(object, object) not throw.

As @jnm2 suggested, I'd have both of these Add methods throw for invalid input (IIRC, when the key is neither a string nor a Type instance). The other methods don't throw because the static typing already guarantees valid input (as far as I can tell), but if it weren't for that, they should throw, too.

Arguments(IDictionary) vs. Arguments(IEnumerable<KeyValuePair<...>>)

TL;DR: I don't quite understand why the former is needed, so I tend towards the latter.

Arguments(IDictionary values)
// it would be good to have an IEnumerable<KeyValuePair<string, object>> constructor,
// but it'll be ambiguous with IDictionary which is more important here so you can pass
// in both named and typed parameters

I don't quite understand that explanation in the comment.

@jonorossi, if memory serves, you previously stated that you're fine with users having to port their code from non-generic collections (say, System.Collections.Hashtable) to the generic counterparts. If so, why is the Arguments(IDictionary) constructor still needed at all?

Is it so you can pass another Arguments object to Argument's ctor for cloning? If so, why not change the ctor parameter type to Arguments, or let Arguments implement IDictionary<object, object> instead?

Arguments.Empty

TL;DR: I'd also remove Arguments.Empty.

You can't do Arguments.Empty.Add(...)

As it stands, Arguments.Empty is the only Arguments instance that is read-only. This isn't obvious at all since it is of a type that exposes Add methods, and there's no public member whose presence might suggest the possibility of read-only-ness.

I see a few options here:

• Redesign Arguments à la System.Collections.Immutable. Arguments.Empty would then be a valid starting point from which to derive new, non-empty instances via .With(...). This would be a lot of work that might not pay off.

• Keep Arguments.Empty, and add either a public Arguments.IsReadOnly or Arguments.MakeReadOnly() method to make the potential read-only-ness of Arguments instances more transparent. Such a property or method likely won't be needed, so this seems like overkill.

• Drop Arguments.Empty (and isReadOnly, EnsureWritable, etc. along with it). This seems most practical.

  It would be good to design all Windsor APIs such that they never force you to repeatedly do a new Arguments() when you don't actually have any "arguments". In such cases it would be better for Arguments parameters to be optional, or to have a method overload without such a parameter.

Factory methods vs. Add methods vs. ctors

TL;DR: I'd go mainly with factory methods (= named constructors for better disambiguation), and keep only a public parameterless ctor and the "copy/clone constructor". I'd also drop any factory method that accepts only a single item (and use .Add or [] = for these).

new Arguments("A", 1)
    .AddNamed("B", 2);

Arguments.FromNamed("A", 1)
    .AddNamed("B", 2);

Arguments                              // same as 2nd example just formatted differently
    .FromNamed("A", 1)
    .AddNamed("B", 2);

Arguments
    .WithNamed("A", 1)
    .WithNamed("B", 2);

Let's start with the last one.

I don't think we can use WithNamed for both static and instance methods because I think that implies that it doesn't modify the Arguments instance, would you assume it is a pure function?

Possibly, yes. Add would be better for a mutating operation, but that doesn't work well for the static factory method. So perhaps not this option.

One thing that bothers me about all other variants is that there are constructors or factory methods that accept a single item in the form of two parameters, like new Arguments("a", 1). This feels somewhat misleading to me: The class name is a plural, and you have two parameters, yet only one entry gets added. It would be better to drop this ctor and similar factory method and do new Arguments { ["a"] = 1 } instead, which thanks to the indexer should already work. Or new Arguments().Add("a", 1).

Put another way: Regardless of whether you decide on ctors or factory methods, they should always accept a (logical) collection, but not just one single item. To add single items, first new up an empty Arguments instance, then use Add or [] =.

params vs IEnumerable parameter

Arguments AddTyped(params object[] arguments) // this handles one or more objects
// should we have an IEnumerable overload here? Will that just be ambiguous with the object one?

I would actually prefer IEnumerable over params object[]: Any object[] is also a IEnumerable<object>, but not the other way around.

Put differently, anyone can add a params overload as an extension method and have it simply forward to the IEnumerable overload; having it the other way around might incur an unnecessary params array allocation or .ToArray() call.

[jonorossi]

Removing Empty sounds great to me.

@jnm2 Done. Looking at it more now I don't think an Empty field was a good idea, every other type with an Empty field acts like a struct or is immutable once created, and the Resolve overloads always have an overload without an Arguments instance, so no need for an empty one.

Shouldn’t they both throw? Why would it make sense for the user to pass things that Arguments doesn’t understand?

@jnm2 I was referring to the fact that Add(object, object) throws when the key already exists, while Add(IDictionary) updates existing keys.

Thought this one was going to be FromProperties? Not sure what a named property is.

@jnm2 I made it AddNamedProperties(object) (and FromNamedProperties(object)) so it matches the other AddNamed* methods, was trying to be consistent and make it clear the public properties are added using their name not their type, which is probably pretty obvious because type would be silly, especially for an anonymous type. If we make it just AddProperties, then AddNamed(IEnumerable<KeyValuePair<string, object>>) probably should just be Add. Hmm, see next comment.

I'd have both of these Add methods throw for invalid input (IIRC, when the key is neither a string nor a Type instance). The other methods don't throw because the static typing already guarantees valid input (as far as I can tell), but if it weren't for that, they should throw, too.

@stakx yer same as I said above, I agree they should throw for invalid input, but what about the key already existing?

I don't quite understand that explanation in the comment.

if memory serves, you previously stated that you're fine with users having to port their code from non-generic collections (say, System.Collections.Hashtable) to the generic counterparts. If so, why is the Arguments(IDictionary) constructor still needed at all?

@stakx good point, I don't think we went far enough with moving away from IDictionary. I also hadn't really questioned it much before now, but I think we should also stop making Arguments implement IDictionary. Even though much of it behaves like a key/value paired collection it is special.

TL;DR: I'd also remove Arguments.Empty

@stakx The rest of your comments are all valid. With this read-only stuff this pull request was trying to do something that wasn't fully implemented, implementing read-only-ness here currently just confuses things because it isn't supported down the chain. As I mentioned above I've removed it, and it can be restored if/when the rest of the work gets done to support immutable Arguments through the resolution chain, even if we can actually do that.

TL;DR: I'd go mainly with factory methods (= named constructors for better disambiguation), and keep only a public parameterless ctor and the "copy/clone constructor". I'd also drop any factory method that accepts only a single item (and use .Add or [] = for these).

I think the 3 of us are in agreement on this now, we started with them but they disappeared. I didn't quite like the constructors that accepted a single argument when that was changed, then sort of liked how much simpler it looked, but maybe that was the wrong solution, maybe Arguments.Single(k, v) or something could work. I do want to stop going around in circles on this so I do like you suggestion of only having factory methods for collections, and using Add methods and the indexer for single items; we can always add more later.

I would actually prefer IEnumerable over params object[]: Any object[] is also a IEnumerable<object>, but not the other way around.

True. It'd be great if you could apply params to IEnumerable, I know this has been a long standing missing feature. Will make this change because the compiler does allow both methods declare and can resolve between them.

[jonorossi]

With GitHub running today, posting my comments stored away yesterday.

I do want to stop going around in circles on this API so we can get this sorted. Please reply to anything useful in my comment above, but I've put together the full contract with some TODOs of things unconfirmed below. I'm hoping the 3 of us can discuss the rest of this, settle on a definition quickly, I'll implement it, and we can get it merged very soon.

class Arguments
    //TODO: probably don't need this anymore, which methods do we keep?
    //TODO: Count, indexer, Add(object, object)??, Clear,
    //TODO:   Contains(object)?, Contains(str)?, Contains(Type)?, Remove?
    // : IDictionary

    //TODO: means we don't need Clone, just pass into ctor?
    // : IEnumerable<KeyValuePair<object, object>>
{
    Arguments() { }
    Arguments(IEnumerable<KeyValuePair<object, object>> arguments) { }
    Arguments(IEnumerable<DictionaryEntry> arguments) { }

    // IEnumerable

    IEnumerator GetEnumerator() { return null; }

    // ICollection

    int Count { get; }
    object SyncRoot { get; }
    bool IsSynchronized { get; }
    void CopyTo(Array array, int index) { }

    // IDictionary

    object this[object key] { get { return null; } set { } }
    ICollection Keys { get { return null; } }
    ICollection Values { get { return null; } }
    bool IsReadOnly { get { return false; } }
    bool IsFixedSize { get { return false; } }
    void Add(object key, object value) { }
    void Clear() { }
    bool Contains(object key) { return false; }
    void Remove(object key) { }

    // Add instance methods

    //TODO: This replaces the IDictionary non-generic overload:
    Arguments Add(IEnumerable<DictionaryEntry> arguments) { return null; }

    //TODO: Do we even want to suffix these methods with Named/Typed anymore???

    Arguments AddNamed(string key, object value) { return null; }
    Arguments AddNamed(IEnumerable<KeyValuePair<string, object>> arguments) { return null; }
    Arguments AddNamedProperties(object instance) { return null; }

    Arguments AddTyped(Type key, object value) { return null; }
    Arguments AddTyped<TDependencyType>(TDependencyType value) { return null; }
    Arguments AddTyped(IEnumerable<object> arguments) { return null; }

    // From factory methods

    //TODO: we've already got the same ctor, remove?
    static Arguments FromNamed(IEnumerable<KeyValuePair<string, object>> arguments) { return null; }

    //TODO: just name it FromProperties since it is obvious they'll be named?
    static Arguments FromNamedProperties(object instance) { return null; }

    //TODO: probably not commonly used, users can use AddTyped which would also be clearer, remove?
    static Arguments FromTyped(IEnumerable<object> arguments) { return null; }
}

[jnm2]

I think Add should throw if the key already exists because this is how all dictionary-like BCL classes work.

I still lean towards FromProperties because all properties are named properties. FromPropertiesNamed is the only way to stop saying "named properties" and instead talk about how the arguments will be constructed, like AddTyped(Arguments), but I think FromProperties is intuitive enough.

Do we even want to suffix these methods with Named/Typed anymore???

Sure; there's an implied word 'argument' or 'arguments' after these names, so they all make sense to me except for AddNamedProperties. I like AddProperties.

I would remove all interface implementations and methods caused by them. As far as I know it's not a goal for users to use this as a general-purpose dictionary type, right? Interfaces should only be implemented if Arguments is intended to be used by code that doesn't know at compile time that it's working with the concrete type Arguments.

[stakx]

I tried to respond to all TODOs and questions so this post got a bit long. Sorry for that.

//TODO: probably don't need this anymore, which methods do we keep?
//TODO: Count, indexer, Add(object, object)??, Clear,
//TODO:   Contains(object)?, Contains(str)?, Contains(Type)?, Remove?
// : IDictionary

Like @jnm2, I don't think that using Arguments like a general-purpose, queryable dictionary is an important use case. I would drop Count, Add(object, object), Clear, Contains, Remove. I would also drop the interface implementation. But I would keep indexers because...

@stakx [...] I agree [Add] should throw for invalid input, but what about the key already existing?

I agree that Add should throw in that case. It might be useful to have indexers as non-throwing (overwriting) alternatives to the single-item AddNamed & AddTyped methods — and also because they enable the new Arguments { [key] = value } syntax, which seems useful.

//TODO: means we don't need Clone, just pass into ctor?
// : IEnumerable<KeyValuePair<object, object>>

I suggest to steer clear of a Clone method; too reminiscent of ICloneable which has been deprecated for many years. I am undecided between letting Arguments implement IEnumerable<KVP<object, object>> and giving it an additional ctor with an Arguments-typed parameter, but tend slightly towards the latter as that exposes less of Arguments' implementation details.

Arguments(IEnumerable<KeyValuePair<object, object>> arguments) { }
Arguments(IEnumerable<DictionaryEntry> arguments) { }
...
//TODO: we've already got the same ctor, remove?
static Arguments FromNamed(IEnumerable<KeyValuePair<string, object>> arguments) { return null; }

Note that IEnumerable<KVP<string, object>> != IEnumerable<KVP<object, object>>. A Dictionary<string, object> won't be assignable to the latter!

That being said, I think it would be good to keep the factory method and drop the ctor (because that way you can "name" the ctor, giving some additional context on what the parameter means).

(I don't know enough about the IEnumerable<DictionaryEntry> ctor and Add method, they seem weird but I suppose that you included them because they're needed somewhere, so I'll stay quiet now. :)

//TODO: Do we even want to suffix these methods with Named/Typed anymore???

Arguments AddNamed(string key, object value) { return null; }
Arguments AddNamed(IEnumerable<KeyValuePair<string, object>> arguments) { return null; }
Arguments AddNamedProperties(object instance) { return null; }

Arguments AddTyped(Type key, object value) { return null; }
Arguments AddTyped<TDependencyType>(TDependencyType value) { return null; }
Arguments AddTyped(IEnumerable<object> arguments) { return null; }

LGTM. It seems good to not call all of those Add. I'd definitely keep the AddTyped name. In fact I lean towards keeping AddNamed also for consistency. I do agree that AddNamedProperties sounds a little redundant and AddProperties might do, but I think both are fine. (AddPropertiesAsNamed probably disqualifies for being too long, right? :)

You could possibly rename AddNamed's and AddTyped's parameter key to parameterName or parameterType, respectively (or just name and type), for some added clarity about their meaning.

//TODO: just name it FromProperties since it is obvious they'll be named?
static Arguments FromNamedProperties(object instance) { return null; }

I'd say decide on AddNamedProperties' name first, then apply exactly the same name change here (if any).

    //TODO: probably not commonly used, users can use AddTyped which would also be clearer, remove?
static Arguments FromTyped(IEnumerable<object> arguments) { return null; }

Same argument ("users can use Add...") could be made for all other factory methods. I'm not sure I understand why AddX would be clearer than FromX, one looks as clear as the other to me personally. I'd probably tend towards keeping the factory method simply for consistency, but I don't mind either way.

[jonorossi]

Thanks guys, working through the comments now.

I didn't know that Dictionary<TKey, TValue> has 2 different indexers that work completely different. Windsor's code through relies on the indexer returning null if a key does not exist.

var dict = new Dictionary<object, object>();

IDictionary a = dict;
IDictionary<object, object> b = dict;

var c = a["hi"]; // returns null
var d = b["hi"]; // throws KeyNotFoundException

Not going to rely on casting to IDictionary, will implement it using TryGetValue.

[jonorossi]

• Renamed to AddProperties and FromProperties
• Removed implementing IDictionary
• Pushed Arguments a little further in the Windsor API
• Implemented IEnumerable<KeyValuePair<object, object>> otherwise you can't use a collection initialiser
• Key/value constructors are gone:

new Arguments("parameter", "Hello") // <- they allowed this
new Arguments().AddNamed("parameter", "Hello") // <- you now need to do
Arguments.Named("parameter", "Hello") // <- Just thought of this idea after seeing the
                                      //    registration fluent interface again, you could just
                                      //    chain .Named and .Typed method calls. It isn't very
                                      //    clear though to use as a C# statement and not very
                                      //    idiomatic C#, whereas the registration API is always
                                      //    used fluently

• Only 2 constructors, default and the copy constructor accepting an Arguments
• Kept Contains, Count and Remove because they are used, and obviously the indexer
• Made all Add methods throw rather than overwrite, I thought there would be heaps of unit tests fail, but not a single one. At least it isn't common in our test code, it leaves it open for someone to write an AddOrReplace extension method.
• Removed Add(IEnumerable<DictionaryEntry>), I added it to replace the IDictionary one to use with .Cast<DictionaryEntry>, however the one place this would get used in Windsor I've replaced with a loop.
• Updated docs

Probably time we wrap this up, please take another look before we merge.

[jnm2]

it leaves it open for someone to write an AddOrReplace extension method.

This is traditionally just indexer assignment which it looks like you already have: dict[newOrExisting] = overwritingValue

Looks great!

[jonorossi]

This is traditionally just indexer assignment which it looks like you already have: dict[newOrExisting] = overwritingValue

Oh definitely, and I expect most people would use the indexer for that. I was referring to the fact you can't use it fluently and if someone does really need that ability (which I think won't be often) they can always implement that extension themselves.

[stakx]

LGTM! Just too minor details I noticed, see review comments.

[jonorossi]

I've squished my commits, time to merge.

Many thanks @jnm2 and @stakx!