diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index dbeef41c..f3cee829 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -10,5 +10,7 @@ jobs: - name: Set up Ruby uses: ruby/setup-ruby@v1 with: + ruby-version: '3.3' + bundler: latest bundler-cache: true - run: bundle exec rubocop diff --git a/.rubocop.yml b/.rubocop.yml index 65778662..54b3ba51 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,9 +1,10 @@ inherit_from: .rubocop_todo.yml -require: +plugins: - rubocop-performance - rubocop-rake - rubocop-rspec + - rubocop-exception_messages AllCops: TargetRubyVersion: 2.7 diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 5b752c20..8806a78a 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -6,6 +6,16 @@ # Note that changes in the inspected code, or installation of new # versions of RuboCop, may require this file to be generated again. +# Remaining offenses after safe autocorrection with modern RuboCop. +Naming/PredicateMethod: + Exclude: + - 'lib/slack/events/request.rb' + - 'lib/slack/utils/security.rb' + +Style/DirectiveScope: + Exclude: + - 'lib/slack/utils/security.rb' + # Offense count: 1 # This cop supports unsafe auto-correction (--auto-correct-all). Lint/NonDeterministicRequireOrder: @@ -107,7 +117,7 @@ RSpec/ExampleLength: # Offense count: 10 # Configuration parameters: Include, CustomTransform, IgnoreMethods, SpecSuffixOnly. # Include: **/*_spec*rb*, **/spec/**/* -RSpec/FilePath: +RSpec/SpecFilePathFormat: Exclude: - 'spec/slack/version_spec.rb' - 'spec/slack/web/api/endpoints/custom/auth_spec.rb' @@ -163,7 +173,8 @@ RSpec/SubjectStub: # Configuration parameters: . # SupportedStyles: constant, string RSpec/VerifiedDoubleReference: - EnforcedStyle: string + Exclude: + - 'spec/slack/web/faraday/request_spec.rb' # Offense count: 2 # Configuration parameters: IgnoreNameless, IgnoreSymbolicNames. diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000..2ce7e893 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,13 @@ +# Contributor guidance + +Read [CONTRIBUTING.md](CONTRIBUTING.md) first for setup, tests, lint, changelog, pull request, and API patch instructions. Keep those instructions there rather than duplicating them here. + +- Preserve the supported-Ruby test matrix when changing lint dependencies. +- When upgrading lint tooling, review safe corrections and baseline only remaining unrelated offenses. +- Exception message corrections can change public error strings. Update matching specs, including handwritten specs under `spec/slack/web/api/endpoints/custom/`. +- Use lowercase exception messages without a trailing period, including messages passed to `super` in error constructors; the exception-message cops may not detect that form. Preserve upstream Slack error codes and dynamically supplied messages. +- Load RuboCop extensions through `plugins:`. When upgrading an extension, migrate renamed cops and obsolete parameters, review safe corrections, and preserve existing behavior rather than blindly applying unsafe fixes. +- Do not edit the API-reference submodule or include existing submodule changes unless requested. +- The API update task pulls the latest reference and can generate unrelated changes. Verify the patch against both the latest and pinned reference; keep unrelated generated updates out of a focused PR. Custom endpoint specs are handwritten despite their location under `endpoints/`. +- Do not suppress environment warnings in application code or tests. Report local validation limitations explicitly; CLI specs compare subprocess output and can fail when Bundler emits unrelated warnings. +- Keep the test bundle on JSON below 3 until Faraday's JSON middleware supports JSON 3's keyword-only parse options. diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c27ec83..26162892 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ### 3.3.0 (Next) +* [#603](https://github.com/slack-ruby/slack-ruby-client/pull/603): Enforce consistent exception messages with rubocop-exception_messages and run lint separately from the test matrix - [@dblock](https://github.com/dblock). * [#602](https://github.com/slack-ruby/slack-ruby-client/pull/602): Make gli an optional dependency; only the `slack` command-line client needs it - [@corsonknowles](https://github.com/corsonknowles). * [#599](https://github.com/slack-ruby/slack-ruby-client/pull/599): Set a default filename in `files_upload` so Slack displays image previews correctly when none is specified - [@ts-3156](https://github.com/ts-3156), [@dblock](https://github.com/dblock). * [#591](https://github.com/slack-ruby/slack-ruby-client/pull/591): Generate AI CHANGELOG entries and PR summaries for automated API update PRs, lock simplecov below 1.1.0 to avoid breaking Coveralls - [@dblock](https://github.com/dblock). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fa030319..746bdbd7 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -59,7 +59,16 @@ We definitely appreciate pull requests that highlight or reproduce a problem, ev Implement your feature or bug fix. -Ruby style is enforced with [Rubocop](https://github.com/bbatsov/rubocop). Run `bundle exec rubocop` and fix any style issues highlighted, auto-correct issues when possible with `bundle exec rubocop -a`. To silence generally ignored issues, including line lengths or code complexity metrics, run `bundle exec rubocop --auto-gen-config`. +Ruby style is enforced with [RuboCop](https://github.com/rubocop/rubocop), including [exception message conventions](https://github.com/dblock/rubocop-exception_messages). The main Gemfile includes lint dependencies only on Ruby 3.2+ so tests can continue running on older supported Ruby versions. + +```bash +bundle install +bundle exec rubocop +``` + +Fix any style issues highlighted, auto-correcting safe issues with `bundle exec rubocop -a`. Exception message corrections require `-A`; review them and update message expectations in tests. To regenerate the baseline for generally ignored issues, including line lengths or code complexity metrics, run `bundle exec rubocop --auto-gen-config`. + +On Ruby 3.2+, `bundle exec rake rubocop` also runs lint. The default `bundle exec rake` runs tests only; CI runs lint separately on Ruby 3.3. Make sure that `bundle exec rake` completes without errors. diff --git a/Gemfile b/Gemfile index 10f799d4..1e800584 100644 --- a/Gemfile +++ b/Gemfile @@ -13,15 +13,12 @@ group :test do gem 'erubis' gem 'faraday-typhoeus' gem 'gli' + gem 'json', '< 3' # Faraday's JSON middleware passes options as a positional hash. gem 'json-schema' gem 'mutex_m' gem 'racc' gem 'rake', '~> 13' gem 'rspec' - gem 'rubocop', '1.26.1' # Lock to specific version to avoid breaking cops/changes - gem 'rubocop-performance' - gem 'rubocop-rake' - gem 'rubocop-rspec' # Lock below 1.1.0, which started writing float timestamps to # coverage/.resultset.json, breaking coverallsapp/github-action's parser # (coverallsapp/github-action#269, coverallsapp/coverage-reporter#191). @@ -31,3 +28,13 @@ group :test do gem 'vcr' gem 'webmock' end + +if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('3.2') + group :lint do + gem 'rubocop', '~> 1.72' + gem 'rubocop-exception_messages', '~> 0.2.0', require: false + gem 'rubocop-performance', '~> 1.27' + gem 'rubocop-rake', '~> 0.7' + gem 'rubocop-rspec', '~> 3.10' + end +end diff --git a/Rakefile b/Rakefile index fc718dae..5757b9b3 100644 --- a/Rakefile +++ b/Rakefile @@ -12,10 +12,14 @@ RSpec::Core::RakeTask.new(:spec) do |spec| spec.pattern = FileList['spec/**/*_spec.rb'] end -require 'rubocop/rake_task' -RuboCop::RakeTask.new +desc 'Run RuboCop (Ruby 3.2+).' +task :rubocop do + abort 'RuboCop requires Ruby 3.2 or newer' if Gem::Version.new(RUBY_VERSION) < Gem::Version.new('3.2') -task default: %i[spec rubocop] + sh 'bundle exec rubocop' +end + +task default: :spec load 'tasks/git.rake' load 'tasks/web.rake' diff --git a/examples/files_upload_v2/files_upload_v2.rb b/examples/files_upload_v2/files_upload_v2.rb index 6b16ba2b..ae94ec5e 100644 --- a/examples/files_upload_v2/files_upload_v2.rb +++ b/examples/files_upload_v2/files_upload_v2.rb @@ -3,7 +3,7 @@ require 'securerandom' Slack.configure do |config| - config.token = ENV['SLACK_API_TOKEN'] + config.token = ENV.fetch('SLACK_API_TOKEN', nil) raise 'Missing ENV[SLACK_API_TOKEN]!' unless config.token end diff --git a/examples/hi_web/hi.rb b/examples/hi_web/hi.rb index 9c0d5903..a886f97b 100644 --- a/examples/hi_web/hi.rb +++ b/examples/hi_web/hi.rb @@ -2,7 +2,7 @@ require 'slack-ruby-client' Slack.configure do |config| - config.token = ENV['SLACK_API_TOKEN'] + config.token = ENV.fetch('SLACK_API_TOKEN', nil) raise 'Missing ENV[SLACK_API_TOKEN]!' unless config.token end diff --git a/examples/new_ticket/new_ticket.rb b/examples/new_ticket/new_ticket.rb index 560e3328..7b719f60 100644 --- a/examples/new_ticket/new_ticket.rb +++ b/examples/new_ticket/new_ticket.rb @@ -2,7 +2,7 @@ require 'slack-ruby-client' Slack.configure do |config| - config.token = ENV['SLACK_API_TOKEN'] + config.token = ENV.fetch('SLACK_API_TOKEN', nil) raise 'Missing ENV[SLACK_API_TOKEN]!' unless config.token end diff --git a/examples/oauth_v2/oauth_v2.rb b/examples/oauth_v2/oauth_v2.rb index 21b6b8fc..8302ac99 100755 --- a/examples/oauth_v2/oauth_v2.rb +++ b/examples/oauth_v2/oauth_v2.rb @@ -18,8 +18,8 @@ response = client.oauth_v2_access( req.query.merge( - client_id: ENV['SLACK_CLIENT_ID'], - client_secret: ENV['SLACK_CLIENT_SECRET'], + client_id: ENV.fetch('SLACK_CLIENT_ID', nil), + client_secret: ENV.fetch('SLACK_CLIENT_SECRET', nil), grant_type: 'authorization_code' ) ) @@ -47,10 +47,10 @@ end query = { - client_id: ENV['SLACK_CLIENT_ID'], - redirect_uri: ENV['REDIRECT_URI'], - scope: ENV['SCOPE'], - user_scope: ENV['USER_SCOPE'] + client_id: ENV.fetch('SLACK_CLIENT_ID', nil), + redirect_uri: ENV.fetch('REDIRECT_URI', nil), + scope: ENV.fetch('SCOPE', nil), + user_scope: ENV.fetch('USER_SCOPE', nil) } url = "https://slack.com/oauth/v2/authorize?#{query.to_query}" diff --git a/lib/slack/events/config.rb b/lib/slack/events/config.rb index 0cc84b91..7cc2da1b 100644 --- a/lib/slack/events/config.rb +++ b/lib/slack/events/config.rb @@ -12,7 +12,7 @@ module Config attr_accessor(*Config::ATTRIBUTES) def reset - self.signing_secret = ENV['SLACK_SIGNING_SECRET'] + self.signing_secret = ENV.fetch('SLACK_SIGNING_SECRET', nil) self.signature_expires_in = 5 * 60 end end diff --git a/lib/slack/web/api/errors/server_error.rb b/lib/slack/web/api/errors/server_error.rb index b84044d2..40d21855 100644 --- a/lib/slack/web/api/errors/server_error.rb +++ b/lib/slack/web/api/errors/server_error.rb @@ -8,7 +8,7 @@ class ServerError < ::Faraday::Error def initialize(message, response) @response = response - super message + super(message) end end diff --git a/lib/slack/web/api/errors/slack_error.rb b/lib/slack/web/api/errors/slack_error.rb index 92e4e03e..cbf20af9 100644 --- a/lib/slack/web/api/errors/slack_error.rb +++ b/lib/slack/web/api/errors/slack_error.rb @@ -7,7 +7,7 @@ class SlackError < ::Faraday::Error attr_reader :response def initialize(message, response = nil) - super message + super(message) @response = response end diff --git a/lib/slack/web/api/errors/too_many_requests_error.rb b/lib/slack/web/api/errors/too_many_requests_error.rb index 58b00cdc..fa62045d 100644 --- a/lib/slack/web/api/errors/too_many_requests_error.rb +++ b/lib/slack/web/api/errors/too_many_requests_error.rb @@ -8,7 +8,7 @@ class TooManyRequestsError < ::Faraday::Error def initialize(response) @response = response - super "Retry after #{retry_after} seconds" + super("retry after #{retry_after} seconds") end def retry_after diff --git a/lib/slack/web/api/helpers/files.rb b/lib/slack/web/api/helpers/files.rb index 699f7529..55fda578 100644 --- a/lib/slack/web/api/helpers/files.rb +++ b/lib/slack/web/api/helpers/files.rb @@ -45,12 +45,12 @@ def files_upload_v2(params = {}) files_to_upload.each_with_index do |file, index| %i[filename content].each do |param| - raise ArgumentError, "Required argument :#{param} missing in file (#{index})" if file[param].nil? + raise ArgumentError, "required argument :`#{param}` missing in file (`#{index}`)" if file[param].nil? end end channel_params = %i[channel channels channel_id].map { |param| params[param] }.compact - raise ArgumentError, 'Only one of :channel, :channels, or :channel_id is required' if channel_params.size > 1 + raise ArgumentError, 'only one of :channel, :channels, or :channel_id is required' if channel_params.size > 1 complete_upload_request_params = params.slice(:initial_comment, :blocks, :thread_ts) diff --git a/lib/slack/web/api/mixins/conversations.id.rb b/lib/slack/web/api/mixins/conversations.id.rb index e2f8b835..3af93fc6 100644 --- a/lib/slack/web/api/mixins/conversations.id.rb +++ b/lib/slack/web/api/mixins/conversations.id.rb @@ -7,6 +7,7 @@ module Api module Mixins module Conversations include Ids + # # This method returns a channel ID given a channel name. # @@ -24,7 +25,7 @@ module Conversations # containing one or more of public_channel, private_channel, mpim, im def conversations_id(options = {}) name = options[:channel] - raise ArgumentError, 'Required arguments :channel missing' if name.nil? + raise ArgumentError, 'required arguments :channel missing' if name.nil? id_for( key: :channel, diff --git a/lib/slack/web/api/mixins/users.id.rb b/lib/slack/web/api/mixins/users.id.rb index a2a8bb58..01828727 100644 --- a/lib/slack/web/api/mixins/users.id.rb +++ b/lib/slack/web/api/mixins/users.id.rb @@ -7,6 +7,7 @@ module Api module Mixins module Users include Ids + # # This method returns a user ID given a user name. # @@ -19,7 +20,7 @@ module Users # The page size used for users_list calls required to find the user's ID def users_id(options = {}) name = options[:user] - raise ArgumentError, 'Required arguments :user missing' if name.nil? + raise ArgumentError, 'required arguments :user missing' if name.nil? id_for( key: :user, diff --git a/lib/slack/web/api/mixins/users.search.rb b/lib/slack/web/api/mixins/users.search.rb index f771f1be..1e50db0b 100644 --- a/lib/slack/web/api/mixins/users.search.rb +++ b/lib/slack/web/api/mixins/users.search.rb @@ -14,7 +14,7 @@ module Users # Free-formed text to search for. def users_search(options = {}) query = options[:user] - raise ArgumentError, 'Required arguments :user missing' if query.nil? + raise ArgumentError, 'required arguments :user missing' if query.nil? index = Picky::Index.new(:users) do category :name diff --git a/spec/slack/web/api/endpoints/custom/auth_spec.rb b/spec/slack/web/api/endpoints/custom/auth_spec.rb index 7d398dc5..40dc9d55 100644 --- a/spec/slack/web/api/endpoints/custom/auth_spec.rb +++ b/spec/slack/web/api/endpoints/custom/auth_spec.rb @@ -20,7 +20,7 @@ it 'fails with an specific exception' do client.auth_test rescue Slack::Web::Api::Errors::TooManyRequestsError => e - expect(e.message).to eq('Retry after 3600 seconds') + expect(e.message).to eq('retry after 3600 seconds') expect(e.retry_after).to eq(3600) end end diff --git a/spec/slack/web/api/endpoints/custom/files_spec.rb b/spec/slack/web/api/endpoints/custom/files_spec.rb index b86b4029..b2e77507 100644 --- a/spec/slack/web/api/endpoints/custom/files_spec.rb +++ b/spec/slack/web/api/endpoints/custom/files_spec.rb @@ -40,7 +40,7 @@ it 'raises an argument error' do expect do client.files_upload_v2(params) - end.to raise_error ArgumentError, /Required argument :#{arg} missing/ + end.to raise_error ArgumentError, /required argument :`#{arg}` missing/ end end end @@ -61,7 +61,7 @@ filename: 'test.txt', content: 'Test File Contents' }.merge(permutation.map { |arg| [arg, 'C08AZ76CA4V'] }.to_h)) - end.to raise_error ArgumentError, 'Only one of :channel, :channels, or :channel_id is required' + end.to raise_error ArgumentError, 'only one of :channel, :channels, or :channel_id is required' end end @@ -73,7 +73,7 @@ channels: 'C08AZ76CA4V', channel_id: 'C08AZ76CA4V' ) - end.to raise_error ArgumentError, 'Only one of :channel, :channels, or :channel_id is required' + end.to raise_error ArgumentError, 'only one of :channel, :channels, or :channel_id is required' end %i[channel channels].each do |arg| @@ -205,7 +205,7 @@ ], channel_id: 'C04KB5X4D' ) - end.to raise_error ArgumentError, /Required argument :filename missing in file \(0\)/ + end.to raise_error ArgumentError, /required argument :`filename` missing in file \(`0`\)/ end end @@ -219,7 +219,7 @@ ], channel_id: 'C04KB5X4D' ) - end.to raise_error ArgumentError, /Required argument :content missing in file \(0\)/ + end.to raise_error ArgumentError, /required argument :`content` missing in file \(`0`\)/ end end end diff --git a/spec/slack/web/api/helpers/files_spec.rb b/spec/slack/web/api/helpers/files_spec.rb new file mode 100644 index 00000000..3f134f2a --- /dev/null +++ b/spec/slack/web/api/helpers/files_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Slack::Web::Api::Helpers::Files do + subject(:client) { Slack::Web::Client.new } + + describe '#files_upload_v2' do + it 'requires a filename' do + expect { client.files_upload_v2(content: 'hello') }.to( + raise_error(ArgumentError, 'required argument :`filename` missing in file (`0`)') + ) + end + + it 'requires content' do + expect { client.files_upload_v2(filename: 'hello.txt') }.to( + raise_error(ArgumentError, 'required argument :`content` missing in file (`0`)') + ) + end + + it 'identifies the invalid file in a batch' do + files = [{ filename: 'hello.txt', content: 'hello' }, { content: 'world' }] + expect { client.files_upload_v2(files: files) }.to( + raise_error(ArgumentError, 'required argument :`filename` missing in file (`1`)') + ) + end + + it 'rejects conflicting channel arguments' do + expect do + client.files_upload_v2(filename: 'hello.txt', content: 'hello', channel: '#general', channel_id: 'C123456') + end.to raise_error(ArgumentError, 'only one of :channel, :channels, or :channel_id is required') + end + end +end diff --git a/spec/slack/web/api/mixins/conversations_spec.rb b/spec/slack/web/api/mixins/conversations_spec.rb index 87704239..0bda11f2 100644 --- a/spec/slack/web/api/mixins/conversations_spec.rb +++ b/spec/slack/web/api/mixins/conversations_spec.rb @@ -24,6 +24,12 @@ end describe '#conversations_id' do + it 'requires a channel' do + expect { conversations.conversations_id }.to( + raise_error(ArgumentError, 'required arguments :channel missing') + ) + end + it 'leaves channels specified by ID alone' do expect(conversations.conversations_id(channel: 'C123456')).to( eq('ok' => true, 'channel' => { 'id' => 'C123456' }) diff --git a/spec/slack/web/api/mixins/users_spec.rb b/spec/slack/web/api/mixins/users_spec.rb index 426eccba..8ec5bc90 100644 --- a/spec/slack/web/api/mixins/users_spec.rb +++ b/spec/slack/web/api/mixins/users_spec.rb @@ -25,6 +25,10 @@ end context '#users_id' do + it 'requires a user' do + expect { users.users_id }.to raise_error(ArgumentError, 'required arguments :user missing') + end + it 'leaves users specified by ID alone' do expect(users.users_id(user: 'U123456')).to eq('ok' => true, 'user' => { 'id' => 'U123456' }) end @@ -69,6 +73,10 @@ if defined?(Picky) context '#users_search' do + it 'requires a search query' do + expect { users.users_search }.to raise_error(ArgumentError, 'required arguments :user missing') + end + it 'finds a user' do expect(users.users_search(user: 'aws')).to( eq('ok' => true, 'members' => [{ 'id' => 'UDEADBEEF', 'name' => 'aws', 'profile' => {} }]) diff --git a/spec/slack/web/client_spec.rb b/spec/slack/web/client_spec.rb index bc9bc7e1..1365093c 100644 --- a/spec/slack/web/client_spec.rb +++ b/spec/slack/web/client_spec.rb @@ -153,7 +153,7 @@ end it 'creates a connection with a logger' do - expect(client.send(:connection).builder.handlers).to include ::Faraday::Response::Logger + expect(client.send(:connection).builder.handlers).to include Faraday::Response::Logger end end end diff --git a/spec/slack/web/faraday/response/raise_error_spec.rb b/spec/slack/web/faraday/response/raise_error_spec.rb index 975e365e..916de376 100644 --- a/spec/slack/web/faraday/response/raise_error_spec.rb +++ b/spec/slack/web/faraday/response/raise_error_spec.rb @@ -14,17 +14,17 @@ context 'with status of 429' do let(:status) { 429 } let(:env) do - env = ::Faraday::Env.from({ - request_headers: { - 'Authorization' => 'Bearer very-secret-token-12345' - }, - response_headers: { - 'retry-after' => 10 - }, - status: status - }) - - env[:response] = ::Faraday::Response.new(env) + env = Faraday::Env.from({ + request_headers: { + 'Authorization' => 'Bearer very-secret-token-12345' + }, + response_headers: { + 'retry-after' => 10 + }, + status: status + }) + + env[:response] = Faraday::Response.new(env) env end @@ -118,16 +118,16 @@ } end let(:env) do - env = ::Faraday::Env.from({ - response_body: body, - request_headers: { - 'Authorization' => 'Bearer very-secret-token-12345', - 'User-Agent' => 'Test Client' - }, - status: status - }) - - env[:response] = ::Faraday::Response.new(env) + env = Faraday::Env.from({ + response_body: body, + request_headers: { + 'Authorization' => 'Bearer very-secret-token-12345', + 'User-Agent' => 'Test Client' + }, + status: status + }) + + env[:response] = Faraday::Response.new(env) env end