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.

Sign up for GitHub

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

Remove hard dep on Redis and update bin #96

Merged
merged 5 commits into from
Jan 8, 2017
Merged

Remove hard dep on Redis and update bin #96

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a5539f5
make redis a dependenc in dev, but only require it to use the backend
Ch4s3 Jan 6, 2017
ff79526
cleaned up the bin files
Ch4s3 Jan 6, 2017
41da6b1
rm bayes.rb
Ch4s3 Jan 7, 2017
8d9a09f
custom raise error
Ch4s3 Jan 7, 2017
e1d6060
Update no_redis_error.rb
Ch4s3 Jan 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,2 @@
source 'https://rubygems.org'
gemspec

gem 'redis'
1 change: 1 addition & 0 deletions classifier-reborn.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,5 @@ Gem::Specification.new do |s|
s.add_development_dependency('minitest-reporters')
s.add_development_dependency('rubocop')
s.add_development_dependency('pry')
s.add_development_dependency('redis')
Copy link
Member

Choose a reason for hiding this comment

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

This works too. 😄

end
6 changes: 5 additions & 1 deletion lib/classifier-reborn/backends/bayes_redis_backend.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
require 'redis'
begin
require 'redis'
rescue LoadError
puts 'The redis gem is required to use the redis backend.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use $stderr instead? Unless, this repository as a whole only uses STDOUT for all sorts of outputs.

$stderr.puts "The redis gem is required to use the Redis backend. To install redis gem run:\n\ngem install redis\n"

Copy link
Member Author

Choose a reason for hiding this comment

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

Either this, or we should raise if it fails... hum

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think raising exception with helpful message would be a better way. It will automatically use the STDERR channel and will allow the application terminate they way it should be.

Copy link
Member

Choose a reason for hiding this comment

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

With more complex systems, I would recommend indicating where this comes from. This message should contain classifier-reborn somewhere in there.

Additionally, the user experience of wanting redis and not fulfilling the requirements should be considered. I think a hard raise would be the most direct way to ensure if the user wants to use redis, they get redis.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll update this to raise with a message including the gem name and maybe some relevant info. I'll play around with error messages and see if I can come up with something intuitive and helpful.

end

module ClassifierReborn
class BayesRedisBackend
Expand Down