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

Making cloudprint gem threadsafe. #4

Merged
merged 2 commits into from Mar 14, 2014

Conversation

Projects
None yet
3 participants
@nasa42
Contributor

nasa42 commented Mar 11, 2014

I've made changes across the application that make it usable in multi-threaded environments.

@thegengen

This comment has been minimized.

Owner

thegengen commented Mar 11, 2014

Hi. Thanks so much for this! I was planning on working on the gem this week, so this is highly appreciated.

However, there's a couple of changes I'd like to see in this pull request.

For one thing, I think CloudPrint::Base should just be called CloudPrint::Client. I think the fact you're calling it client everywhere in the tests is a good hint that's what the class should be called too.

Secondly, I don't like the ActiveSupport dependency. It's not that I don't like ActiveSupport itself, it's just that it doesn't seem to be offering us much here. I see a few calls to delegate (which could just as well be defined as methods), a few calls to .present? (which aren't really that necessary), and that line containing to_options! in CloudPrint::Printer#initialize which I'd rather not have in the codebase at all.

Thirdly, since we're breaking the API, let's get rid of CloudPrint.setup() entirely and tell people to use CloudPrint::Client.new(...) instead.

Other than that, this looks very good. I can write up the documentation changes myself, as long as you make these adjustments. And, again, thanks!

@theinventor

This comment has been minimized.

theinventor commented Mar 11, 2014

Great feedback, I second

* Removed activesupport dependency.
* Renamed CloudPrint::Base to CloudPrint::Client.
* Removed CloudPrint.setup in favour of CloudPrint::Client.new.
@nasa42

This comment has been minimized.

Contributor

nasa42 commented Mar 12, 2014

It has some pretty good test coverage helping me immensely in rewriting the application. So, thanks for them! :-)
Implemented suggest changes.

@thegengen

This comment has been minimized.

Owner

thegengen commented Mar 13, 2014

This looks very good. I'm going to set up Travis integration for this project today, and then I'll merge it. Thanks so much!

thegengen added a commit that referenced this pull request Mar 14, 2014

Merge pull request #4 from webstream-io/master
Make cloudprint gem threadsafe.

@thegengen thegengen merged commit f55f407 into thegengen:master Mar 14, 2014

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