robotlegs / robotlegs-framework

An ActionScript 3 application framework for Flash and Flex
https://robotlegs.tenderapp.com/
MIT License
967 stars 261 forks source link

RL2 Mediator Matching #62

Closed darscan closed 12 years ago

darscan commented 12 years ago

There are two options for mediator view matching:

  1. Any hamcrest matcher you like.
  2. Specialized/restricted Robotlegs matcher.

    Option 1 (Arbitrary Object Matching)

Allows a great deal of flexibility, but there are concerns that if offers too much in the way of "rope".

Pros

  1. Flexible - any matcher you can think up.
  2. Work done - a full suite of tested matchers already exist in the Hamcrest library.
  3. Consistent - other parts of the Robotlegs API use the Hamcrest API for matching.
  4. Feature parity with Option 2 - the TypeMatcher used as a restriction in Option 2 can still be used with this approach.

    Cons

  5. Perhaps too flexible - concerns that developers may do silly things.
  6. Harder to tool - arbitrary matchers might not provide enough information to help the mediator factory decide what types to map the view instance to, for example.

    Option 2 (Specialized Type Matching)

Restricts usage by employing a custom matcher interface tailored for "type matching". This reduces the "rope" factor, but is less flexible.

Pros

  1. Prevents developer mistakes.
  2. Can be optimised for internal use by the mediator factory.

    Cons

  3. At odds with the rest of the RL2 API.
  4. Less flexible.

    Your thoughts?

Personally I'm keen for Option 1 - even if purely to keep the RL2 API consistent.

Which option are you in favour of?

Stray commented 12 years ago

I wholeheartedly prefer Option 2.

Option 1 allows you to create brittle and hard-to-reason code that works sometimes and is unpredictable at runtime - with the unexpected behaviours arising in exactly the kinds of ways that are hard to unit test.

For example, you could supply a Hamcrest matcher for mediators like:

allOf ( instanceOf (SomeColouredView), hasProperty('color', 0x009900) )

Whether or not an instance of SomeColouredView is mediated now depends on its color, which is dynamic - so you come to depend on order of operations:

var view:ISomeView = new SomeColouredView(0x009900);
addChild(view);
view.color = 0x000099;

vs

var view:ISomeView = new SomeColouredView(0x009900);
view.color = 0x000099;
addChild(view);

In the first instance the view is mediated, in the second it isn't.

That may not be, in itself, impossible to reason - but what happens when the view is reparented?

Currently we pause and immediately resume mediator activity during reparenting (in response to consecutive removedFromStage and addedToStage events).

Where mediation is only by type / package (or anything else which is fixed at compile time), this is consistent with what would happen were I to remove the view and then add it to a new view in 2 separate lines of code (rather than simply using the fact that I've added it to a different parent to do the removal for me).

In case number 1 above, the view is eligible for mediation at the time when it is first added to the stage, but not eligible during reparenting (because it no longer matches). Should it continue to be mediated when it is reparented, even though doing an explicit removeChild immediately followed by addChild would cause it to stop being mediated?

Worse - I think it's reasonable that users might expect that this matching is 'live' - that changing the color on a view will result in it being / not being mediated, regardless of its stage presence - this is not what we can support, so we shouldn't make it look like we support it. We can only support inspection at the moment the view hits the stage, so we should only map on the basis of immutable things (within the scope of the validity of the mapping).

If we want to do composition-based mediation, similar to how entity systems work, we can explore the right approach for implementing that in an immutable (and type safe) way, but leaving the API open to any Hamcrest matcher is too much flexibility in my mind. I'd be much more comfortable with requiring only those things which have been actively considered (and tested) when building the framework.

To be clear: advanced users are already free to write their own alternative implementation of any Type or Package (or even Composition) matcher we supply. We're not nailing anything shut in not allowing arbitrary matching - we're just not having our API appear to require something that is much looser than what it actually requires in order to get the tested behaviour.

On 'not being consistent':

map in the EventCommandMap requires a String (and a Class if you like). In the MessageCommandMap it requires an Object ... so I don't know what API convention we're breaking.

It's also possible that there are also other places where simply requiring a Hamcrest matcher isn't really robust enough. Can you list out the other places where it's in the API? I couldn't easily find them when browsing.

On 'Work Done':

We also already have done (and tested) this work - in both ways - so that's not really a difference between the two.


My thoughts in a nutshell:

Configuration of the 80% functionality provided by the framework should be designed to funnel new users into easily reasoned, robust code that has a low propensity to explode in unexplained ways at runtime. For our sanity (as forum support) as well as for theirs. When we surveyed RL1 users they did not say "it's not flexible enough" - they consistently said that they liked the strong prescription and wanted more, not less, of a groove to hook into.

But - I'm interested to hear people's thoughts - perhaps I'm overly protective / pessimistic.

astronaute77 commented 12 years ago

Even if more freedom and flexibility seems attractive, I think Stray has valid points: "we should only map on the basis of immutable things" & "advanced users are already free to write their own alternative implementation".

So I vote for Option 2.

joelstransky commented 12 years ago

I think the success of RL1 was heavily due to its consistent rewards throughout a projects lifetime. While the entry fee was low, its ability to handle large changes and new features with grace is what kept me coming back. My point is that I suspect most users just used it as is and those with the need an capability to alter it found it reasonable. I vote for option 2 as that will allow for advancement down the road and still reward new users while option 1 may force you to un-ring a bell at some point.

darscan commented 12 years ago

Fair enough. I'm happy to let the community make the final call on this one. Let's leave this open and get some more opinions in.

@Stray Matchers are used by the core of the framework: The ObjectProcessor, ConfigManager and in turn the Context itself. The switch to Hamcrest was made while developing those components, when I realised that a custom solution was a duplication of effort. However, as the MediatorMap is an extension I'm OK with breaking consistency and using something tailored to mediation.

Stray commented 12 years ago

@darscan - yes, I should admit that I currently am unclear about how / why / when a user would use the 2 bits of the api that the matchers are in at the moment. What's the task / problem they're solving there?

Don't you think the map() api is - at the point of use - a closer mental fit with mapping commands to triggers? (Ignoring the under the hood mechanics).

neilmanuell commented 12 years ago

Could you use the Iface in Option 2 to wrap a Hamcrest matcher (Option 1)?

alebianco commented 12 years ago

I think the "map to immutable things" point is the winning one and, beside that, if we want to check variable properties before mapping a mediator, we could add a guard, right? Also, probably a dedicated API would be more readable that a sequence of matchers.

definitely option 2 for me

Stray commented 12 years ago

@neil - yes, you could use hamcrest under the hood in your custom matcher.

neilmanuell commented 12 years ago

@Stray could it restrict the use of a Hamcrest matcher so that developers can't do the "silly things" that they could do?

mikecann commented 12 years ago

Agreed on Strays points. Alot of people ask 'whats the proper way to do X' if the response is 'well there are any number of ways' then it can scare people.

Stray commented 12 years ago

@neilmanuell a custom requirement (which may be a special case of hamcrest matcher) can do anything it likes. The option here is between:

  1. Allow any old Hamcrest matcher.
  2. Only accept our custom interface.

What it can't do is enforce, for example, the 'instanceOf' Hamcrest matcher - because the whole beauty of covariant mediation is that you can combine rules - so to build an equivalent rule in Hamcrest you'd often be submitting an allOf() or anyOf() matcher, which then permits you to populate that matcher with any kind of matcher you like.

Hamcrest itself is really a side issue - the issue is whether we should steer people towards making mappings only on the basis of (immutable) type, package (and possibly composition), or whether we should facilitate dynamic object property mappings.

darscan commented 12 years ago

@mikecann The answer to that question would usually be: "Use the provided TypeMatcher". It's just that Option 1 allows more specialized/optimised use for developers who know exactly what they want.

hyakugei commented 12 years ago

I'm on board with Option 2, for the same reasons stated by the previous posters.

To reiterate, matching static properties and making it easier for new users of RL2 to understand it are my key points. It seems like RL2 is going to offer the opportunity to extend this via plugins/extensions anyway, for those with more specific needs.

neilmanuell commented 12 years ago

Yep, Option Two, if you need more dynamic matching create an extention

darscan commented 12 years ago

@Stray

yes, I should admit that I currently am unclear about how / why / when a user would use the 2 bits of the api that the matchers are in at the moment. What's the task / problem they're solving there?

Heh, they pretty much are the framework :) End users might never touch those bits directly themselves, but they are core to the framework's architecture and used by extension authors or end users who want to hook in to the framework initialization process.

Don't you think the map() api is - at the point of use - a closer mental fit with mapping commands to triggers? (Ignoring the under the hood mechanics).

Very good point! That may sway it for me (or at least make me a little less sad about the whole thing).

Stray commented 12 years ago

@darscan - Sorry, I should have been clearer - I can't figure out how a normal, 80% (or even 90%) user, ever comes in to contact with those parts of the API.

But it sounds like possibly they don't?

Obviously they're in action under the hood - but I took API to mean the parts that normal users use in their apps. Stuff that would be covered in your typical "How to use Robotlegs" guide. (Assuming that this guide doesn't consider extension building which I think is advanced and more likely to be in the 10%/20% usage group).

I don't think under-the-hood-API and typical-user-facing-API are necessarily connected.

richardlord commented 12 years ago

This is my third attempt to write my opinion here. Every time, I change my mind before I get to the end. Shaun, would you be so kind as to ask an easy question next time ;).

Stray's point about mapping to immutable things makes a lot of sense and my initial instinct is that she's right to favour option 2. I think that Robotleg's users are (or should be) looking for a flexible architecture and an implementation that enforces, or at least encourages, best practices. In that context a custom mapping implementation that effectively limits users to the equivalent of Hamcrest's anyOf(), instanceOf() and isInterface() matchers sounds compelling.

But then, favouring composition over inheritance is a good rule, so matching on composition would also be good. So maybe it needs some sort of hasChild() in there too (along with allOf()) and that's dynamic, although events are dispatched when it changes so maybe that's manageable.

And then... oh I don't know. What about practical examples. Can anyone come up with a real, dynamic mapping that doesn't have a 'better', non-dynamic alternative? Is that a valid question?

Or, which is better, when a user asks "Why doesn't this work?" and you can say "Because your dynamic property changes after the mediator has been mapped" or when a user asks "Why can't I do this?" and you have to say "Because we didn't think you'd want to."

And Shaun, yes I'd be sad too. Using Hamcrest to provide matching for mediators in Robotlegs is a lovely idea. If I was developing Robotlegs it's what I'd want to do. Heck, if I'm selfish it is what I want you to do.

And that's just some of my thoughts. I'm not convinced, or convincing, either way.

darscan commented 12 years ago

Score! I get a half a +1, finally. ;-)

Stray commented 12 years ago

@richardlord

CompositionMatcher is something I think would be lovely and had already looked at implementing. But members (the definition of them in a class) are immutable. I think that's entirely safe. (Wouldn't include hasChild() in that though - but as you can just mediate the child directly... maybe that's a more common solution? After all, our mediatorMap already supports composition of behaviour by mediating children as they come and go.)

Did you see the existing TypeMatcher? That does anyOf(Classes...) allOf(Classes...) noneOf(Classes..) - so you can get a lot of flexibility.

There would never be a "Because we didn't think you'd want to..." answer. The answer would be "Here's how to build your own matcher wrapper that also meets our interfaces, with the following health-warnings about what might cause problems."

In my view, pain is potentially the developer's friend. We should make it more painful to do something potentially unstable, vs doing something that has been fully tested. (All sorts of inappropriate analogies come to mind so I'll leave it there!)

richardlord commented 12 years ago

@Stray

Yes, the existing TypeMatcher is probably all I would ever need anyway. I think the aesthete in me wants option 1 and the pragmatist wants option 2. It's probably best to listen to the pragmatist. He is, after all, being pragmatic. Sorry Shaun.

darscan commented 12 years ago

Damnit, there goes my half +1. The aesthete stands alone.

Stray commented 12 years ago

@darscan

"The aesthete stands alone."

Yeah, but he looks good while he does it ;)

joelhooks commented 12 years ago

I'm with Stray on this one. The core shipping framework providing concrete guidance (albiet with plenty of rope, as my consulting on RL gigs has proven to me) is important.

+1 option 2

darscan commented 12 years ago

@Stray haha!

Ok, to my great surprise it seems like Option 2 is the way to go. The aesthetes must all be off somewhere, preoccupied with code formatting and whatnot :)

actionscriptor commented 12 years ago

You've been announced that RL2 will be so flexible, that Option 1 is the one way to go -) But I vote for 2nd one… cause it closer to me. As @Stray said before «To be clear: advanced users are already free to write their own alternative implementation of any Type or Package (or even Composition) matcher we supply» and that's it!

darscan commented 12 years ago

Thanks to everyone for participating. Luckily for me, my vote counts as a +20, so I still win. Just kidding. We'll go with Option 2.