Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates #67

Merged
merged 4 commits into from
Sep 14, 2014
Merged

Updates #67

merged 4 commits into from
Sep 14, 2014

Conversation

creynders
Copy link
Contributor

Hi @geekdave a number of fixes, and improvements:

  1. I fixed the double line as mentioned by @mmikeyy
  2. Optimized the applyToConstructor function
  3. Added a factory/constructor method for views as proposed in Views #50 which means you can use injected views either as a factory function or as a constructor function
  4. Fully refactored Geppetto. This simply means I merged the Resolver object into the Context. The separation made sense when we started out with the DI, but it had become a weird mess with some wire* methods hanging directly on the context others in resolver etc. In fact, two classes weren't necessary since they were tightly coupled anyway.

If you don't like any of this either cherry-pick the ones you do or let me know and I'll do it for you.

@creynders
Copy link
Contributor Author

Ah, I didn't have the courage to merge the spec files. Which would be a bad idea anyway, since they're becoming huge. I think it would be best to fully refactor the tests as well and split them up into (a) pure unit test(s) of the API and some functionality tests.

@creynders creynders mentioned this pull request Sep 12, 2014
@creynders
Copy link
Contributor Author

@geekdave So what do you think? Could you Shall I 😁 cut a patch release? Everything's backwards compatible, but maybe the added API methods warrant a minor release?

I didn't bother with adding the view factories to the readme yet, since a full rewrite of the docs is one of the first things that needs to happen now IMO.

@geekdave
Copy link
Contributor

@creynders Looks great - I think it's patch release time. Please feel free to do the honors!

@creynders creynders merged commit e5bf09c into GeppettoJS:master Sep 14, 2014
@creynders
Copy link
Contributor Author

@geekdave 0.7.1 is fully prepared and tagged. But. I don't have permissions to publish it on npm. Could you add me with

npm owner add creynders backbone.geppetto

?

@geekdave
Copy link
Contributor

@creynders : I just published 0.7.1 to npm, and also added you to the maintainers.

@creynders
Copy link
Contributor Author

@geekdave thanks! 🍻

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.

None yet

2 participants