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

add an early return to not connect to the servers #26

Merged

Conversation

blindgaenger
Copy link
Contributor

… if there is nothing to write or delete

This is needed, because if the model doesn't have an image, we still rely on the asset servers.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.48% when pulling ae291a9 on blindgaenger:early_return_without_connecting_the_servers into fc38376 on xing:master.

end
end.each(&:join)
end.each(&:join)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Instead of wrapping the code in a huge unless block, I would prefer to just use an early return in the first line of the method:

return if @queued_for_write.empty?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I now see that you still want to call after_flush_writes. Then the suggestion I just made obviously does not work.

@blindgaenger
Copy link
Contributor Author

Yep, exactly! I wanted to keep the after_flush_writes callback. And for consistency reasons I've added the block for writes and deletes in the same manner.

So should I use an early return in flush_deletes?

boosty added a commit that referenced this pull request Mar 31, 2015
…ing_the_servers

add an early return to not connect to the servers
@boosty boosty merged commit be3b177 into xing:master Mar 31, 2015
@boosty
Copy link
Contributor

boosty commented Mar 31, 2015

I think keeping the code consistent is ok.
Thanks again for the PR!
Pushed paperclip-storage-ftp 1.2.4 to rubygems.org.

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