Changing Definition creation API to be more compact

[alexgarbarev]

Ideally I want

- (void)injectProperty:(SEL)withSelector;
- (void)injectProperty:(SEL)selector with:(id)something;
- (void)injectProperty:(SEL)withSelector withStringRepresentation:(NSString *)string;
- (void)injectProperty:(SEL)withSelector withCollection: (...)

Where 'something' in method

- (void)injectProperty:(SEL)selector with:(id)something;

can be:

• definition
• object instance
• factory ( self reference in assembly subclass)

New methods:

withDefinition:(TyphoonDefinition *)factoryDefinition selector:(SEL)factorySelector;
withDefinition:(TyphoonDefinition *)factoryDefinition keyPath:(NSString *)keyPath;

should be removed and replaced by something like (method names are bad, let's think more)

- (TyphoonDefinition *) resultOfInvocationSelector:(SEL)selector;
- (TyphoonDefinition *) resultOfInvocationKeypath:(NSString *)string;

I.e.

[definition injectProperty:@selector(property) with:[[self factoryDefinition] resultOfInvocation:@selector(newInstance)];

[jasperblues]

I can see the merit in making it more compact, however I was a bit hesitant about:

• Upgrading users will have to make an (albeit trivial) change to their assembly
• Anytime we use type 'id' we need documentation to describe what it can be. (user will have to click-through to read the header).
• There's a little bit of surprise/inconsistency - we just inject the object. But give special treatment to a definition. surprise!

Still . . unless someone will raise a red flag, I say let's do it.

[jasperblues]

Benefits:

• More compact and fluent. Less to remember.
• Speaking of surprises, some users have used 'injectWithDefinition' in place of 'injectWithObjectInstance' and vice-verse, and gotten an equally unpleasant surprise. This fixes that.

[alexgarbarev]

I tried to implement that I want: compact API as possible. Finally I got API:
Note: "" - temroary prefix to not duplicate current API methods_

@interface TyphoonDefinition (ExperimentalAPI)

- (void)_injectProperty:(SEL)selector;
- (void)_injectProperty:(SEL)selector with:(id)injection;

- (id)_injectionFromSelector:(SEL)factorySelector;
- (id)_injectionFromKeyPath:(NSString *)keyPath;

@end

id InjectionFromStringRepresentation(NSString *string);
id InjectionFromCollection(void (^collection)(TyphoonPropertyInjectedAsCollection *collectionBuilder));

Here is example of usage:

//object
[self _injectProperty:@selector(property) with:@"object"];

//number value    
[self _injectProperty:@selector(property) with:@(3.1415f)];

//definition    
[self _injectProperty:@selector(property) with:[self anotherDefinition]];

//assembly (i.e. ComponentFactory )
[self _injectProperty:@selector(property) with:self];

//proxy assembly (i.e. ComponentFactory )
[self _injectProperty:@selector(property) with:self.collaboratingAssembly];

//NSValue struct    
[self _injectProperty:@selector(property) with:[NSValue valueWithRange:NSMakeRange(0, 20)]];

//Result of invoking factory method on resolved definition    
[self _injectProperty:@selector(property) with:[[self anotherDefinition] _injectionFromSelector:@selector(factoryMethod)]];

// Value from string representation    
[self _injectProperty:@selector(property) with:InjectionFromStringRepresentation(@"3.1415")];

//Collection
[self _injectProperty:@selector(property) with:InjectionFromCollection(^(TyphoonPropertyInjectedAsCollection *collectionBuilder) {
    [collectionBuilder addItemWithDefinition:[self anotherDefinition]];
    [collectionBuilder addItemWithText:@"3.1415" requiredType:[NSNumber class]];
})];

I'm not sure that InjectionFromCollection and InjectionFromStringRepresentation functions was good idea - I just tried to have only one main method for property injection.

[alexgarbarev]

I didn't test my implementation yet (but I think it should works :-) ) because I want to discuss result API first.

[jasperblues]

Looks very nice. . overall there's less to remember. And it reads better.

👍

[alexgarbarev]

Does anyone else have an opinion on this API?

[drodriguez]

Just one comment. I haven’t look at the code “in place” but from the diff it seems to be something internal, not destined for the final user, right? If so, we should define InjectionFromStringRepresentation and InjectionFromCollection as static in the *.m file, or have them with the Typhoon prefix we use everywhere, to avoid clashes with user functions. I think someone commented using a macro like TYPHOON_SHORTHAND (similar to OCHamcrest one) to avoid verbose APIs, but to allow namespace in case it is needed.

Other than that, nothing else to comment.

[jasperblues]

@drodriguez 👍

[alexgarbarev]

I agree about functions, but if we do shorthand we loose IDE autocompletion for function arguments (for example collection builder block)..

[jasperblues]

Between losing auto-complete and seeing the word 'Typhoon' in front, I'd probably choose the latter. .

[alexgarbarev]

Finally I did really compact API. Without any C functions.. just - (void)injectProperty:(SEL)selector with:(id)injection; and - (void)injectParameterWith:(id)injection;
And also runtime arguments now available for definitions!
The results now available in branch runtime-args, I'll be happy with any feedback
@jasperblues you can test in your project now. (I think now API is stable.. )

[jasperblues]

This is über-cool!!!! Great work alekesy! 🎱

[alexgarbarev]

Results merged to master. This issue can be closed after review

[eriksundin]

I had a look at it. I like it, really clean. And I agree it will be much simpler for the end-user.
Runtime arguments .. nice. Great work.

Only thing, minor one:
Typhoon class headers are missing on the new files added.
Code samples inline in the class header files (ex. TyphoonDefinition.h) need to be updated.

Probably a good idea to create a separate issue for updating the Wiki for this rather big change before the next release.

[alexgarbarev]

Thanks.
Sure, headers should be updated with comments (haven't time for this yet).
Also quick question: should we keep documentation seprate from code? I mean short description in code, detailed explanation in separate place (documentation site), to keep headers more clean and clear for quick review. Currently I had to scroll through few screens between methods declarations

[jasperblues]

@eriksundin , I've added a Typhoon 2.0 release checklist #193

[jasperblues]

All good as far as I'm concerned. One comment: TyphoonConfig assumes the prefix for a TyphoonPropertyPlaceholderConfigurer is the default ${} . . . this is probably fine.

[eriksundin]

@jasperblues Checklist. 👍

I can agree that documentation takes up a lot of space in some API classes. Although, the nice thing that come out of it is the generated documentation on http://www.typhoonframework.org/docs/latest/api/modules.html.
However, it requires a lot of discipline to keep documentation updated. And, in a team everyone needs to be aware of what needs to be updated and how. If it becomes too much of a problem in the long run I'd opt to rethink it. I'm good with keeping it as is right now.

[jasperblues]

The nice thing about the API docs also, is that they're indexed by Google, unlike the wiki pages in Github. . . . So I can type: TyphoonAssembly just in Google and get the docs, which I like.

The rendering in the IDE looks a little cluttered. I read that Xcode5 is supposed to render the markup in to rich-text - not sure how it works. Same feature is coming in AppCode.

• Examples: TyphoonAssembly (reasonably brief) and TyphoonFactoryProvider (long, but very good)
• Enough information to refresh you. . . perhaps links on where to find more information?

[alexgarbarev]

Ok. Thanks for comments. Let's keep as is.

[jasperblues]

If no comments from @drodriguez or @rhgills , lets close. . . waiting 48 hours.

[drodriguez]

👍

[rhgills]

Looks great to me. Good work alekesy! 👍