Skip to content

Fix tests for #54 and add missing ones for full coverage#61

Merged
geekdave merged 3 commits intoGeppettoJS:masterfrom
creynders:fix_54
Sep 4, 2014
Merged

Fix tests for #54 and add missing ones for full coverage#61
geekdave merged 3 commits intoGeppettoJS:masterfrom
creynders:fix_54

Conversation

@creynders
Copy link
Copy Markdown
Contributor

Hey @mmikeyy I hope you don't mind but I modified the tests to make #54 pass. Sorry, just eager to getting a patch release and definitely wanted to squeeze your changes in too.

@geekdave I added a few uncovered testing paths as well, no major stuff, just keeping that percentage up there.

mmikeyy and others added 3 commits August 28, 2014 14:54
context.parentContext just needs to be a reference to the parent context.

createChildResolver function not required. No need to go to the parent context to do anything as the parent has no need to be aware of the child's existence (it's the child that must reference the parent).

Original createChildResolver function was calling "child" a resolver based on the parent context as this._context is the parent context when the function is called.
@mmikeyy
Copy link
Copy Markdown
Contributor

mmikeyy commented Sep 4, 2014

Ahh! good! I had asked if you wanted me to change the tests because it involved removing existing tests concerning the createChildResolver function. Just wanted to make sure you guys were ok with the changes before devoting too much time to this. But I'm totally happy if you did the tests!

BTW, I'm so busy with a conversion to Geppetto that I've undertaken lately that I'm having a hard time keeping up with the discussions here. But if fixed parent contexts are to be included in the patch release, perhaps there would be an interest in a little but extremely useful function function I've added and am using a lot. I call it dispatchToParents and the difference with the existing dispatchToParent is that it bubbles up events. I could probably find time today to add it with some tests if you think it's a good idea for this patch release. With working parent contexts, it's just something one would expect to be available, IMHO. For now, it's just a plain vanilla thing, but later features could be added such as the possibility to stop propagation somewhere along the context inheritance chain, or the possibility to start dispatching on current context instead of parent. But for now, the basic implementation is ultra simple an it is very useful.

geekdave added a commit that referenced this pull request Sep 4, 2014
Fix tests for #54 and add missing ones for full coverage
@geekdave geekdave merged commit 587b9e8 into GeppettoJS:master Sep 4, 2014
@geekdave
Copy link
Copy Markdown
Contributor

geekdave commented Sep 4, 2014

Looks great, @creynders ! Any more changes you want to get in before we create a new release?

@mmikeyy : With regard to bubbling events to multiple levels of parents, I think it could be useful. Please feel free to create a PR for this. There is some history/discussion on that here as well: #34

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants