Changing authority of Optional/Required Addons

[mxgrey]

In the current implementation of "optionality" for Addons, it is an Addon that decides for itself whether it is optional for a given Manager type. This is semantically backwards, since the Manager owns the Addon and should therefore have authority over whether the Addon is considered optional or required.

This pull request reverses this relationship. Now a Manager can specify which Addons are required for it in a similar fashion to how Addons can be specialized for it. In fact, declaring an Addon as "required" will also automatically specialize the Addon for that Manager.

[mxgrey]

I'm kind of wondering if we should change the name of the class SpecializedAddonManager<~> to SpecializedForAddon<~>. That would be more consistent with the class named RequiresAddon<~>. Same thing for SpecializedNodeManager.

[jslee02]

The overall looks good.

I have similar concern about the names though. The class name RequiresAddon sounds intuitive to use, but I'm not sure if having verb in class name (e.g., RequiresAddon) is a good practice. At the same time, I can't come up with better alternative for now. So I would go with RequiresAddon unless someone has alternative name for it.

Also, if we don't have any alternative then changing SpecializedAddonManager to SpecializedForAddon seems good for the consistency.

[mxgrey]

I think a verb makes sense in a class name if the class itself does not stand alone. If its purpose is to augment a more complete type, then a verb name would be good for expressing this role (IMO).

We would never have an object that is simply a RequiresAddon<T>. Instead, we would always place RequiresAddon<T> in an inheritance list where it could be thought of as expressing a characteristic of the class (the characteristic that the class requires an Addon of type T).

[mkoval]

I generally agree with @jslee02's argument that class names should be nouns. However, in this case, I agree with @mxgrey. Changing RequiresAddon<T> to RequiredAddon<T> makes it sound like the subclass is a T addon, not that it contains a required T addon.

I think we should stick with the verb names. Like @jslee02 suggested, we should also change SpecializedAddonManager<T> to SpecializedForAddon<T> for consistency.

[jslee02]

Okay, RequiresAddon<T> sounds good then.

@mxgrey Can you change SpecializedAddonManager<T> to SpecializedForAddon<T> if you are also fine with it?

[mxgrey]

The renaming has now been pushed.

[jslee02]

Thanks! Will merge once Travis-CI tests are passed.

[psigen]

Just chiming in here: I believe it's pretty common for mixins to have verb/adjective naming, since they are often being used to describe properties that an existing class is taking on as a result of the inheritance rather than defining a class itself. This naming makes sense under that convention.