Add qualifier to disambiguate injection

[Todderz]

Hello,

I've been going around in circles debating the right way to do the injection. In the end I've scrapped most of the ideas I had, aiming at simplicity.

I had favoured injection by name, but I think your decision to go for injection by type was a good one. Whether by name or by type, eventually you run into some odd cases that require special treatment, but for everything else injection by type allows a clean and simple solution that requires minimal configuration by users, is predictable and unsurprising, and efficient.

Your choice of policy for the injection is actually quite convincing and creates the simplicity and consistency:

 *    If a mock can be assigned to a field, do it. The same mock an be assigned more than once
 *    If no mock can be assigned to a field, skip it silently
 *    If two mocks can be assigned to the same field, return an error

The biggest problem I have with it is the last line, because it disallows using a different mock for each of two fields with the same type.

The simplest way I can see to get around this is to allow an optional "qualifier" element in the Mock annotation, to identify the target field for the injection.

To implement it without making a mess of deeply nested conditionals, I've reorganised the code into some new classes.

First I've pulled your original injection code into a new Injector class. The first real change in the code is that instead of passing a list of mock Objects, we now pass a list of a new Injection class, which holds the mock Object and Mock annotation that describes it.

When we come to injectMocksOnClass, we enlist another new type, InjectionTarget, just to keep things clean.

When checking each mock to see if it can be assigned to the field, we have a slight variation to account for the qualifier:-

 *    If there is no qualifier, we do the same as before, including rejecting duplicate injection candidates.
 *    If there is a qualifier, but not matching this field, we just skip this one: It can't be used here.
 *    If there is a qualifier, we set this mock as the assignment candidate, rejecting duplicates with the same qualifier, but allowing a qualified mock to displace an unqualified mock.

I've added tests for these few cases: Qualifiers distinguish two fields of the same type; A qualified mock displaces an unqualified mock; Duplicate qualifiers on the same type are rejected.

The original injection policy is still true, and all the previous behaviour is preserved, but we add one extra line at the end to clarify what we mean where the first line says "If a mock can be assigned to a field":-

 *    A mock can be assigned if the type is compatible and, where a qualifier is set, the qualifier matches the field name

For your consideration!

Al

[henri-tremblay]

Keep it simple is a good idea. It needs to stay intuitive.

I think that when a qualifier is used, it should match or throw an exception. I can't see cases where one would like a qualifier set but unused.

Also, maybe the qualifier should be called "fieldName" to be obvious (does it still fit if we allow to inject on setters?)

The would bring the following rules:
Start with mock with a field name:

• If a mock has a field name, assign it do that field
• If two mocks have the same field name, return an error
• If a mock has a field name and no matching field is found, return an error
  Then, ignoring all fields and mocks matched y field name
• If a mock without field name can be assigned to a field, do it. The same mock can be assigned more than once
• If no mock can be assigned to a field, skip it the field silently
• If two mocks can be assigned to the same field, return an error

WDYT?

[Todderz]

Yes, I like that. "fieldName" is a more directly obvious name. Treating an unmatched fieldName as an error is better for avoiding behaviour that is hard to debug. Separately explaining the behaviour for qualified and unqualified injection makes it clearer.

I'll make those changes.

To clarify the case where two mocks can be assigned to the same field - in the first draft, if a qualified mock and an unqualified mock can be assigned to the same field, the qualified mock is assigned. Sounds like you want this to be an error, and I think I now agree, i.e. if two mocks, whether qualified or unqualified or a combination, can be assigned to the same field, return an error.

Seems simpler that way. Wherever there is a possibility to assign two mocks to the same field, it's an error.

[henri-tremblay]

It would want that everything qualified be set first. And then the unqualified ones. So if you have two fields with the same type, you only have to qualify only one.

The one thing I would like to avoid is ambiguity. I don't want a field to be assigned randomly to the first matching mock the algorithm found.

[Todderz]

Ah, OK, cool. "Ignoring all fields and mocks matched by field name" is the vital piece I didn't fully grasp. When looking for places to assign unqualified mocks we exclude any fields already assigned a qualified mock.

[henri-tremblay]

Exactly

On 3 November 2013 18:03, Alistair Todd notifications@github.com wrote:

Ah, OK, cool. "Ignoring all fields and mocks matched by field name" is the
vital piece I didn't fully grasp. When looking for places to assign
unqualified mocks we exclude any fields already assigned a qualified mock.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3#issuecomment-27648655
.

[henri-tremblay]

If it feels unclear, feel free to add precisions

On 3 November 2013 18:57, Henri Tremblay henri.tremblay@gmail.com wrote:

Exactly

On 3 November 2013 18:03, Alistair Todd notifications@github.com wrote:

Ah, OK, cool. "Ignoring all fields and mocks matched by field name" is
the vital piece I didn't fully grasp. When looking for places to assign
unqualified mocks we exclude any fields already assigned a qualified mock.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3#issuecomment-27648655
.

[Todderz]

Created a new pull request for an improved solution. This one can be closed.