-
Notifications
You must be signed in to change notification settings - Fork 109
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
Remove hard dep on Redis and update bin #96
Conversation
@ibnesayeed What are your thoughts on making Redis a soft dependency? This gem gets rolled into Jekyll and I imagine people don't want to run redis to build their blogs. |
@Ch4s3 I like this soft dependency approach better. I was actually thinking about it in the beginning that if there is a way to get the tests passing, but don't force users to install a dependency that they might not use. However, I didn't know the trick to provision that. |
begin | ||
require 'redis' | ||
rescue LoadError | ||
puts 'The redis gem is required to use the redis backend.' |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -1,4 +1,2 @@ | |||
source 'https://rubygems.org' | |||
gemspec | |||
|
|||
gem 'redis' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW this is how you'd do a soft dependency for this repo. It'd be a hard dependency if it were added to the .gemspec
file instead as a runtime_dependency
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I thought it made sense to keep it in the gemspec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way to convey the message is to use conditional inclusion in the Gemfile like:
gem 'redis' if ENV['REDIS']
This approach will still need the NoRedisError
message, but the text can be updated to reflect the procedure or passing environment variable while running bundle install
. This will have an added advantage of tracking the desired version number of the gem in the Gemfile. Additionally, anyone looking at the Gemfile will notice the need of the gem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather avoid using ENV vars for installation. That can make things more complicated for users, and libs upstream that use Classifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather avoid using ENV vars for installation. That can make things more complicated for users, and libs upstream that use Classifier.
I agree! However, I have seen this trick being used in SciRuby/rb-gsl/Gemfile.
@@ -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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works too. 😄
@@ -6,31 +6,3 @@ | |||
rescue | |||
require 'classifier' | |||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not remove the whole file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wanted to double check and make sure it wasn't being used.
I added a custom error class, but the use feels clunky. What do you @parkr and @ibnesayeed? begin
require 'redis'
rescue LoadError
raise NoRedisError
end Which emits this error:
|
There are actually two requirements to use the Redis backend:
If the gem is installed but the Redis server is not available to connect then the error should be raised from the redis gem itself. You might want to update the message to educate users about the two requirements. |
I could probably just wrap the redis error and return separate info |
I was thinking of catching that error in the redis backend if the connection can't be established with the redis server (which might not be running or the correct connection config is not passed). But then I realized that the underlying redis gem will issue sensible message, enough for the user to know what's wrong. Here is the exception that is raised from redis gem.
If this is not enough (which I think it actually is) then we can update the initialize method of BayesRedisBackend with custom message. |
That probably works. |
I would avoid specifying the version number here (assuming that we dont have any specific need of a specific version of redis gem and the user is familiar enough with the Gemfile to specify a version if desired). Having a version number here means it will be an ongoing maintenance overhead, one which would be very easy to forget about (who will regularly check the upstream?). This might cause users to believe that we strictly need that version even if the upstream deprecates that version. |
Good point |
I was going to have minitest fire off 'redis-server' in a process before tests and kill it at the end, but with #101 that seems pointless, so I'm calling this done! |
No description provided.