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

Adding serviceFn #104

Merged
merged 1 commit into from
Jan 26, 2018
Merged

Adding serviceFn #104

merged 1 commit into from
Jan 26, 2018

Conversation

ragboyjr
Copy link
Contributor

  • Added new serviceFn function to create service functions
    instead of classes
  • Refactored service into serviceFactory to share code between
    serviceFn and service.

Signed-off-by: RJ Garcia rj@bighead.net

@ragboyjr
Copy link
Contributor Author

This could resolve #103

@young-steveo
Copy link
Owner

Thanks for the PR!

I think I can be convinced that an "injectable factory" method like serviceFn is a good addition, but I would make a couple of suggestions for this PR:

  • The main difference between this new function and the bottle.factory is that this factory will be injected with dependencies as arguments to the factory instead of having the entire container injected. I think this should be explained in the Readme (as it stands it could be confused that bottle.serviceFn is exactly like bottle.factory)
  • Don't expose serviceFactory internals to the public api. You can leave the bottle.service tests where they were, unchanged, and write some new tests for the serviceFn to show how it's different.
  • I'm not keen on the abbreviated serviceFn name; none of the other bottle api methods use abbreviations, so it makes this one stand out. You could maybe rename it to serviceFactory, and then rename your internal serviceFactory to something else. This will align well with the current instanceFactory method name.

@ragboyjr
Copy link
Contributor Author

Awesome, will do, all of your points sound good. thanks for taking the time to review!

- Added new `serviceFactory` function which injects dependencies
  into functions
- Refactored `service` so that it shares code with `serviceFactory`

Signed-off-by: RJ Garcia <rj@bighead.net>
@ragboyjr
Copy link
Contributor Author

@young-steveo updated.

@young-steveo young-steveo merged commit a9d1964 into young-steveo:master Jan 26, 2018
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.

2 participants