slack-ruby / slack-ruby-client

A Ruby and command-line client for the Slack Web, Real Time Messaging and Event APIs.
MIT License
1.21k stars 214 forks source link

Fix CLI `require` to use relative path for slack ruby client #487

Closed chrisbloom7 closed 1 year ago

chrisbloom7 commented 1 year ago

Updates bin/slack to use a relative path to the slack ruby client to use in environments where the library isn't pre loaded (like outside of tests). Prior to this change running bin/slack from the command line would result in an error:

<internal:/usr/local/rvm/rubies/ruby-3.2.2/lib/ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:85:in `require': cannot load such file -- slack_ruby_client (LoadError)
        from <internal:/usr/local/rvm/rubies/ruby-3.2.2/lib/ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:85:in `require'
        from bin/slack:3:in `<main>'
dangerpr-bot commented 1 year ago
1 Warning
:warning: There're library changes, but not tests. That's OK as long as you're refactoring existing code.

Generated by :no_entry_sign: Danger

dblock commented 1 year ago

Before I just merge, could we add an integration test for this? Thinking something in https://github.com/slack-ruby/slack-ruby-client/tree/master/spec/integration that runs without an environment. We do this in https://github.com/ruby-grape/grape for example.

chrisbloom7 commented 1 year ago

Yup, sounds good. I'm traveling for the next two weeks but will address when I get back

chrisbloom7 commented 1 year ago

Oh, we do already have https://github.com/slack-ruby/slack-ruby-client/blob/19fe79d256c67b1240a85c966aac575c4cd163c2/spec/slack/slack_spec.rb#L5 which exercises that script. To be honest I'm not entirely sure why that test continues to pass prior to this change. Perhaps because commands in backticks are executed in a subprocess and something about bundler or rspec or the test environment makes sure that library is in the load path before hand? Given that, I'm not sure how we'd go about writing a test that would fail prior to the change. Any thoughts?

dblock commented 1 year ago

Take a look at https://github.com/dblock/slack-ruby-client/tree/cli-path, https://github.com/dblock/slack-ruby-client/commit/282c5fba0555f280ddd5f6b7ec30e79d8f852bb3, this test reproduces this problem if you run BUNDLE_GEMFILE=spec/bin/Gemfile bundle install and BUNDLE_GEMFILE=spec/bin/Gemfile bundle exec rspec spec/bin/slack_spec.rb.

$ BUNDLE_GEMFILE=spec/bin/Gemfile bundle exec rspec spec/bin/slack_spec.rb 

Slack CLI
  #help
/Users/dblock/source/slack-ruby/slack-ruby-client/dblock-slack-ruby-client/bin/slack:3:in `require': cannot load such file -- slack_ruby_client (LoadError)
    from /Users/dblock/source/slack-ruby/slack-ruby-client/dblock-slack-ruby-client/bin/slack:3:in `<main>'
    displays help (FAILED - 1)

Failures:

  1) Slack CLI #help displays help
     Failure/Error: expect(help).to include 'slack - Slack client.'
       expected "" to include "slack - Slack client."

But ... looking at this closely, what's the actual scenario "where the library isn't pre loaded"? How do you get the other dependencies such as gli or hashie? Aren't you supposed to require 'slack-ruby-client' at least once somewhere to get everything working, for example via Gemfile? If you add gem 'slack-ruby-client', path: '../..' into the Gemfile I added everything works without these changes.

chrisbloom7 commented 1 year ago

@dblock the scenario is running bin/slack from the command line when developing the gem. The current code require 'slack_ruby_client' assumes that the client library is installed as a Gem, which if you're in a clean environment, like a codespace for example, then it isn't. By changing it to use the relative require then it will go look at the local installation. I suppose using bundle exec bin/slack would be the other way to work around this.

 $ bin/slack 
<internal:/usr/local/rvm/rubies/ruby-3.2.2/lib/ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:85:in `require': cannot load such file -- slack_ruby_client (LoadError)
        from <internal:/usr/local/rvm/rubies/ruby-3.2.2/lib/ruby/3.2.0/rubygems/core_ext/kernel_require.rb>:85:in `require'
        from bin/slack:3:in `<main>'
rbenv: version `2.7.6' is not installed (set by /workspaces/slack-ruby-client/.ruby-version)                                                                                                                                                                                                                        
$ bundle exec bin/slack
NAME
    slack - Slack client.

I doesn't seem to be a problem when used from a binstub when slack_ruby_client is installed into a project. Maybe given that is the most likely scenario for use we can just close this?

dblock commented 1 year ago

I doesn't seem to be a problem when used from a binstub when slack_ruby_client is installed into a project. Maybe given that is the most likely scenario for use we can just close this?

Yeah, the "right way" is bundle exec bin/slack, I think we should not change the require, feels brittle. I'll close it.