Drop giphy dependency #89
Conversation
| @@ -2,5 +2,6 @@ rvm: | |||
| - 2.3.0 | |||
| env: | |||
| - CONCURRENCY=celluloid-io | |||
| - CONCURRENCY=celluloid-io WITH_GIPHY=true | |||
There was a problem hiding this comment.
I'd just call it GIPHY to be consistent with CONCURRENCY, nbd though.
There was a problem hiding this comment.
Thought about that as well, but since CONCURRENCY is a requirement and giphy is optional, I think the WITH_ prefix makes it's purpose a bit more clear.
|
I am down with this change, I think it's a good idea. I'd like it to behave in a more backward compatible fashion. I think saying "include a dependency on giphy and that turns on GIFs" is better than having to tweak environment variables. So keep the environment settings "as is", but just make things work without the dependency as before? Also please update CHANGELOG and UPGRADING. Thanks! |
|
I'm sorry, I don't think I follow. Do you mean to leave GIFs enabled by default, just don't blow up if |
|
Yes. If giphy is installed, everything works as before (aka GIFs are enabled as before, you can turn them off and on via ENV or whatever is already in-place, no or little code should really change here). If giphy is not installed, GIFs are disabled, environment variables have no effect, ideally settings like |
tmsrjs
force-pushed
the
drop_giphy_dependency
branch
from
8ad673a to
21e865d
Compare
| @@ -2,8 +2,6 @@ | |||
|
|
|||
| RSpec::Matchers.define :respond_with_error do |error, error_message| | |||
| match do |actual| | |||
| allow(Giphy).to receive(:random) | |||
There was a problem hiding this comment.
This is going to break downstream users upgrading the library. I think this needs to be put back and have a if defined?(Giphy) or one of those.
|
My only remaining issue is with shared rspec behaviors, sorry I missed this in the first CR. Thanks for hanging on here! |
GIF support is enabled by default if Giphy is installed but we don't want to force it onto users during tests.
|
Looks good, merging, thanks! |
Although GIFs are a nice feature they're not really necessary for all bots.
This PR drops the dependency on
giphy.If
giphyis installed, everything will work as up until now.