From 4979ef9e44aa1bad39d30f9af3ecec2c6df0a283 Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Fri, 5 Jul 2024 15:50:02 +0200 Subject: [PATCH 1/3] Move cops in opf/rubocop-openproject gem It saves 700ms on rspec loading time. --- .rubocop.yml | 4 +- Gemfile | 1 + Gemfile.lock | 9 ++ ...k_with_rspec_capybara_matcher_in_expect.rb | 111 ------------- .../use_service_result_factory_methods.rb | 145 ----------------- ...h_rspec_capybara_matcher_in_expect_spec.rb | 61 -------- ...use_service_result_factory_methods_spec.rb | 146 ------------------ spec/rails_helper.rb | 9 -- 8 files changed, 11 insertions(+), 475 deletions(-) delete mode 100644 lib_static/rubocop/cop/open_project/no_do_end_block_with_rspec_capybara_matcher_in_expect.rb delete mode 100644 lib_static/rubocop/cop/open_project/use_service_result_factory_methods.rb delete mode 100644 spec/lib/rubocop/cop/open_project/no_do_end_block_with_rspec_capybara_matcher_in_expect_spec.rb delete mode 100644 spec/lib/rubocop/cop/open_project/use_service_result_factory_methods_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index 13ea37fa1ef6..799e8c4def87 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,10 +1,8 @@ require: + - rubocop-openproject - rubocop-rails - rubocop-rspec - rubocop-rspec_rails - - ./lib_static/rubocop/cop/open_project/add_preview_for_view_component.rb - - ./lib_static/rubocop/cop/open_project/no_do_end_block_with_rspec_capybara_matcher_in_expect.rb - - ./lib_static/rubocop/cop/open_project/use_service_result_factory_methods.rb - rubocop-capybara - rubocop-factory_bot - rubocop-performance diff --git a/Gemfile b/Gemfile index 0ac360f48639..d419c513275f 100644 --- a/Gemfile +++ b/Gemfile @@ -331,6 +331,7 @@ group :development, :test do gem "rubocop", require: false gem "rubocop-capybara", require: false gem "rubocop-factory_bot", require: false + gem "rubocop-openproject", github: "opf/rubocop-openproject", branch: "main", require: false gem "rubocop-performance", require: false gem "rubocop-rails", require: false gem "rubocop-rspec", require: false diff --git a/Gemfile.lock b/Gemfile.lock index a31df8c7db19..d9d78d0aa236 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -49,6 +49,14 @@ GIT omniauth-openid_connect-providers (0.2.0) omniauth-openid-connect (>= 0.2.1) +GIT + remote: https://github.com/opf/rubocop-openproject.git + revision: d1830d89bb64509fc7ce26103cce04bc9d6b3dc9 + branch: main + specs: + rubocop-openproject (0.1.0) + rubocop + GIT remote: https://github.com/opf/turbo_tests.git revision: c1c4707f536a5642a168650d273d714dfb62d842 @@ -1312,6 +1320,7 @@ DEPENDENCIES rubocop rubocop-capybara rubocop-factory_bot + rubocop-openproject! rubocop-performance rubocop-rails rubocop-rspec diff --git a/lib_static/rubocop/cop/open_project/no_do_end_block_with_rspec_capybara_matcher_in_expect.rb b/lib_static/rubocop/cop/open_project/no_do_end_block_with_rspec_capybara_matcher_in_expect.rb deleted file mode 100644 index 70160c2ff298..000000000000 --- a/lib_static/rubocop/cop/open_project/no_do_end_block_with_rspec_capybara_matcher_in_expect.rb +++ /dev/null @@ -1,111 +0,0 @@ -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) 2012-2024 the OpenProject GmbH -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License version 3. -# -# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -# Copyright (C) 2006-2013 Jean-Philippe Lang -# Copyright (C) 2010-2013 the ChiliProject Team -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -module RuboCop::Cop::OpenProject - # As +do .. end+ block has less precedence than method call, a +do .. end+ - # block at the end of a rspec matcher will be an argument to the +to+ method, - # which is not handled by Capybara matchers (teamcapybara/capybara/#2616). - # - # # bad - # expect(page).to have_selector("input") do |input| - # input.value == "hello world" - # end - # - # # good - # expect(page).to have_selector("input") { |input| input.value == "hello world" } - # - # # good - # expect(page).to have_selector("input", value: "hello world") - # - # # good - # match_input_with_hello_world = have_selector("input") do |input| - # input.value == "hello world" - # end - # expect(page).to match_input_with_hello_world - # - # # good - # expect(foo).to have_received(:bar) do |arg| - # arg == :baz - # end - # - class NoDoEndBlockWithRSpecCapybaraMatcherInExpect < RuboCop::Cop::Base - extend RuboCop::Cop::AutoCorrector - - CAPYBARA_MATCHER_METHODS = %w[selector css xpath text title current_path link button - field checked_field unchecked_field select table - sibling ancestor].flat_map do |matcher_type| - ["have_#{matcher_type}", "have_no_#{matcher_type}"] - end - - MSG = "The `do .. end` block is associated with `to` and not with Capybara matcher `%s`.".freeze - - def_node_matcher :expect_to_with_block?, <<~PATTERN - # ruby-parse output - (block - (send - (send nil? :expect ...) - :to - ... - ) - ... - ) - PATTERN - - def_node_matcher :rspec_matcher, <<~PATTERN - (send - (send nil? :expect...) - :to - (:send nil? $_matcher_method ...) - ) - PATTERN - - def on_block(node) - return unless expect_to_with_block?(node) - return unless capybara_matcher?(node) - - add_offense(offense_range(node), message: offense_message(node)) - end - - private - - def capybara_matcher?(node) - matcher_name = node.send_node.arguments.first.method_name.to_s - CAPYBARA_MATCHER_METHODS.include?(matcher_name) - end - - def offense_range(node) - node.send_node.loc.selector.join(node.loc.end) - end - - def offense_message(node) - rspec_matcher(node.send_node) do |matcher_method| - format(MSG, matcher_method:) - end - end - end -end diff --git a/lib_static/rubocop/cop/open_project/use_service_result_factory_methods.rb b/lib_static/rubocop/cop/open_project/use_service_result_factory_methods.rb deleted file mode 100644 index 5be797405c47..000000000000 --- a/lib_static/rubocop/cop/open_project/use_service_result_factory_methods.rb +++ /dev/null @@ -1,145 +0,0 @@ -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) 2012-2024 the OpenProject GmbH -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License version 3. -# -# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -# Copyright (C) 2006-2013 Jean-Philippe Lang -# Copyright (C) 2010-2013 the ChiliProject Team -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -module RuboCop::Cop::OpenProject - # # bad - # ServiceResult.new(success: true, result: 'result') - # ServiceResult.new(success: false, errors: ['error']) - # - # # good - # ServiceResult.success(result: 'result') - # ServiceResult.failure(errors: ['error']) - # ServiceResult.new(success: some_value) - # ServiceResult.new(**kwargs) - class UseServiceResultFactoryMethods < RuboCop::Cop::Base - extend RuboCop::Cop::AutoCorrector - - MSG = "Use ServiceResult.%s(...) instead of ServiceResult.new(success: %s, ...).".freeze - MSG_IMPLICIT_FAILURE = "Use ServiceResult.failure instead of ServiceResult.new.".freeze - - def_node_matcher :service_result_constructor?, <<~PATTERN - (send - $(const nil? :ServiceResult) :new - ... - ) - PATTERN - - def_node_matcher :constructor_with_explicit_success_arg, <<~PATTERN - (send - (const nil? :ServiceResult) :new - (hash - < - $(pair (sym :success) ({true | false})) - ... - > - ) - ) - PATTERN - - def on_send(node) - return unless service_result_constructor?(node) - - if success_argument_present?(node) - add_offense_if_explicit_success_argument(node) - elsif success_argument_possibly_present?(node) - return # rubocop:disable Style/RedundantReturn - else - add_offense_for_implicit_failure(node) - end - end - - private - - def success_argument_present?(node) - hash_argument = node.arguments.find(&:hash_type?) - return false unless hash_argument - - hash_argument.keys.any? { |key| key.sym_type? && key.value == :success } - end - - def success_argument_possibly_present?(node) - return true if node.arguments.find(&:forwarded_args_type?) - - hash_argument = node.arguments.find(&:hash_type?) - return false unless hash_argument - - hash_argument.children.any?(&:kwsplat_type?) - end - - def add_offense_if_explicit_success_argument(node) - constructor_with_explicit_success_arg(node) do |success_argument| - message = format(MSG, success_value: success_value(success_argument), - factory_method: factory_method(success_argument)) - add_offense(success_argument, message:) do |corrector| - corrector.replace(node.loc.selector, factory_method(success_argument)) - corrector.remove(removal_range(node, success_argument)) - end - end - end - - def add_offense_for_implicit_failure(node) - add_offense(node.loc.selector, message: MSG_IMPLICIT_FAILURE) do |corrector| - corrector.replace(node.loc.selector, "failure") - end - end - - def success_value(success_argument) - success_argument.value.source - end - - def factory_method(success_argument) - success_argument.value.source == "true" ? "success" : "failure" - end - - def removal_range(node, success_argument) - if sole_argument?(success_argument) - all_parameters_range(node) - else - success_parameter_range(success_argument) - end - end - - def sole_argument?(arg) - arg.parent.loc.expression == arg.loc.expression - end - - def all_parameters_range(node) - node.loc.selector.end.join(node.loc.expression.end) - end - - # rubocop:disable Metrics/AbcSize - def success_parameter_range(success_argument) - if success_argument.left_sibling - success_argument.left_sibling.loc.expression.end.join(success_argument.loc.expression.end) - else - success_argument.loc.expression.begin.join(success_argument.right_sibling.loc.expression.begin) - end - end - # rubocop:enable Metrics/AbcSize - end -end diff --git a/spec/lib/rubocop/cop/open_project/no_do_end_block_with_rspec_capybara_matcher_in_expect_spec.rb b/spec/lib/rubocop/cop/open_project/no_do_end_block_with_rspec_capybara_matcher_in_expect_spec.rb deleted file mode 100644 index 0086bbbc75a6..000000000000 --- a/spec/lib/rubocop/cop/open_project/no_do_end_block_with_rspec_capybara_matcher_in_expect_spec.rb +++ /dev/null @@ -1,61 +0,0 @@ -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) 2012-2024 the OpenProject GmbH -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License version 3. -# -# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -# Copyright (C) 2006-2013 Jean-Philippe Lang -# Copyright (C) 2010-2013 the ChiliProject Team -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -require "spec_helper" -require "rubocop/cop/open_project/no_do_end_block_with_rspec_capybara_matcher_in_expect" - -RSpec.describe RuboCop::Cop::OpenProject::NoDoEndBlockWithRSpecCapybaraMatcherInExpect do - include RuboCop::RSpec::ExpectOffense - include_context "config" - - context "when using `do .. end` syntax with rspec matcher" do - it "registers an offense" do - expect_offense(<<~RUBY) - expect(page).to have_selector("input") do |input| - ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ The `do .. end` block is associated with `to` and not with Capybara matcher `have_selector`. - end - RUBY - end - - it "matches only Capybara matchers" do - expect_no_offenses(<<~RUBY) - expect(foo).to have_received(:bar) do |value| - value == 'hello world' - end - RUBY - end - end - - context "when using `{ .. }` syntax with rspec matcher" do - it "does not register an offense" do - expect_no_offenses(<<~RUBY) - expect(page).to have_selector("input") { |input| } - RUBY - end - end -end diff --git a/spec/lib/rubocop/cop/open_project/use_service_result_factory_methods_spec.rb b/spec/lib/rubocop/cop/open_project/use_service_result_factory_methods_spec.rb deleted file mode 100644 index 86520fc44b44..000000000000 --- a/spec/lib/rubocop/cop/open_project/use_service_result_factory_methods_spec.rb +++ /dev/null @@ -1,146 +0,0 @@ -#-- copyright -# OpenProject is an open source project management software. -# Copyright (C) 2012-2024 the OpenProject GmbH -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License version 3. -# -# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows: -# Copyright (C) 2006-2013 Jean-Philippe Lang -# Copyright (C) 2010-2013 the ChiliProject Team -# -# This program is free software; you can redistribute it and/or -# modify it under the terms of the GNU General Public License -# as published by the Free Software Foundation; either version 2 -# of the License, or (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -# -# See COPYRIGHT and LICENSE files for more details. -#++ - -require "spec_helper" -require "rubocop/cop/open_project/use_service_result_factory_methods" - -RSpec.describe RuboCop::Cop::OpenProject::UseServiceResultFactoryMethods do - include RuboCop::RSpec::ExpectOffense - include_context "config" - - it "registers an offense for ServiceResult.new without any success: argument" do - expect_offense(<<~RUBY) - ServiceResult.new - ^^^ Use ServiceResult.failure instead of ServiceResult.new. - ServiceResult.new(errors: ['error']) - ^^^ Use ServiceResult.failure instead of ServiceResult.new. - RUBY - - expect_correction(<<~RUBY) - ServiceResult.failure - ServiceResult.failure(errors: ['error']) - RUBY - end - - it "allows ServiceResult.new(success: some_value) (no explicit true/false value)" do - expect_no_offenses("ServiceResult.new(success: some_value)") - expect_no_offenses('ServiceResult.new(foo: "bar", success: some_value, bar: "baz")') - end - - it "allows ServiceResult.new(**kw) (no explicit true/false value)" do - expect_no_offenses("ServiceResult.new(**kw)") - expect_no_offenses('ServiceResult.new(foo: "bar", **kw)') - expect_no_offenses('ServiceResult.new(**kw, foo: "bar")') - end - - include_context "ruby 3.1" do - it "allows ServiceResult.new(success:) (no explicit true/false value)" do - expect_no_offenses("ServiceResult.new(success:)") - expect_no_offenses('ServiceResult.new(foo: "bar", success:, bar: "baz")') - end - - it "allows ServiceResult.new(...) (no explicit true/false value)" do - expect_no_offenses(<<~RUBY) - def call(...) - ServiceResult.new(...) - end - RUBY - end - end - - it "registers an offense for ServiceResult.new(success: true) with no additional args" do - expect_offense(<<~RUBY) - ServiceResult.new(success: true) - ^^^^^^^^^^^^^ Use ServiceResult.success(...) instead of ServiceResult.new(success: true, ...). - RUBY - - expect_correction(<<~RUBY) - ServiceResult.success - RUBY - end - - it "registers an offense for ServiceResult.new(success: true) with additional args" do - expect_offense(<<~RUBY) - ServiceResult.new(success: true, - ^^^^^^^^^^^^^ Use ServiceResult.success(...) instead of ServiceResult.new(success: true, ...). - message: 'Great!') - ServiceResult.new(message: 'Great!', - success: true) - ^^^^^^^^^^^^^ Use ServiceResult.success(...) instead of ServiceResult.new(success: true, ...). - RUBY - - expect_correction(<<~RUBY) - ServiceResult.success(message: 'Great!') - ServiceResult.success(message: 'Great!') - RUBY - end - - it "registers an offense for ServiceResult.new(success: false) with no additional args" do - expect_offense(<<~RUBY) - ServiceResult.new(success: false) - ^^^^^^^^^^^^^^ Use ServiceResult.failure(...) instead of ServiceResult.new(success: false, ...). - ServiceResult.new success: false - ^^^^^^^^^^^^^^ Use ServiceResult.failure(...) instead of ServiceResult.new(success: false, ...). - RUBY - - expect_correction(<<~RUBY) - ServiceResult.failure - ServiceResult.failure - RUBY - end - - it "registers an offense for ServiceResult.new(success: false) with additional args" do - expect_offense(<<~RUBY) - ServiceResult.new(success: false, - ^^^^^^^^^^^^^^ Use ServiceResult.failure(...) instead of ServiceResult.new(success: false, ...). - errors: ['error']) - ServiceResult.new(errors: ['error'], - success: false) - ^^^^^^^^^^^^^^ Use ServiceResult.failure(...) instead of ServiceResult.new(success: false, ...). - RUBY - - expect_correction(<<~RUBY) - ServiceResult.failure(errors: ['error']) - ServiceResult.failure(errors: ['error']) - RUBY - end - - it "registers an offense for ServiceResult.new(success: true/false) with splat kwargs" do - expect_offense(<<~RUBY) - ServiceResult.new(success: true, **kw) - ^^^^^^^^^^^^^ Use ServiceResult.success(...) instead of ServiceResult.new(success: true, ...). - ServiceResult.new(success: false, **kw) - ^^^^^^^^^^^^^^ Use ServiceResult.failure(...) instead of ServiceResult.new(success: false, ...). - RUBY - - expect_correction(<<~RUBY) - ServiceResult.success(**kw) - ServiceResult.failure(**kw) - RUBY - end -end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 46f28236affd..998cb8959124 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -46,15 +46,6 @@ # https://github.com/paper-trail-gem/paper_trail#7b-rspec require "paper_trail/frameworks/rspec" -# Add rubocop rspec helpers for our cop tests. It needs to be done before RSpec -# requires all files so that the CopHelper module does not hide some let -# definitions. -# -# Ideally we should move our cops to their own gem. -require "rubocop" -require "rubocop/rspec/shared_contexts" -require "rubocop/rspec/support" - # Requires supporting ruby files with custom matchers and macros, etc, in # spec/support/ and its subdirectories. Files matching `spec/**/*_spec.rb` are # run as spec files by default. This means that files in spec/support that end From 94252fb8185ff4562099e9c58551c7bed656340f Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Fri, 5 Jul 2024 16:13:22 +0200 Subject: [PATCH 2/3] Load rubocop-openproject in rubocop github action workflow --- .github/workflows/rubocop-core.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/rubocop-core.yml b/.github/workflows/rubocop-core.yml index d1953e531b68..1cec9bf1086a 100644 --- a/.github/workflows/rubocop-core.yml +++ b/.github/workflows/rubocop-core.yml @@ -19,6 +19,7 @@ jobs: rubocop_extensions: > rubocop-capybara:gemfile rubocop-factory_bot:gemfile + rubocop-openproject:gemfile rubocop-performance:gemfile rubocop-rails:gemfile rubocop-rspec:gemfile From 540a45c080dfff2220841d04eeac746b1c4fef8f Mon Sep 17 00:00:00 2001 From: Christophe Bliard Date: Fri, 5 Jul 2024 16:25:00 +0200 Subject: [PATCH 3/3] Use released rubocop-openproject 0.1.0 --- Gemfile | 2 +- Gemfile.lock | 12 +++--------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/Gemfile b/Gemfile index d419c513275f..60f1c7aefffc 100644 --- a/Gemfile +++ b/Gemfile @@ -331,7 +331,7 @@ group :development, :test do gem "rubocop", require: false gem "rubocop-capybara", require: false gem "rubocop-factory_bot", require: false - gem "rubocop-openproject", github: "opf/rubocop-openproject", branch: "main", require: false + gem "rubocop-openproject", require: false gem "rubocop-performance", require: false gem "rubocop-rails", require: false gem "rubocop-rspec", require: false diff --git a/Gemfile.lock b/Gemfile.lock index d9d78d0aa236..9effb3a5492a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -49,14 +49,6 @@ GIT omniauth-openid_connect-providers (0.2.0) omniauth-openid-connect (>= 0.2.1) -GIT - remote: https://github.com/opf/rubocop-openproject.git - revision: d1830d89bb64509fc7ce26103cce04bc9d6b3dc9 - branch: main - specs: - rubocop-openproject (0.1.0) - rubocop - GIT remote: https://github.com/opf/turbo_tests.git revision: c1c4707f536a5642a168650d273d714dfb62d842 @@ -994,6 +986,8 @@ GEM rubocop (~> 1.41) rubocop-factory_bot (2.26.1) rubocop (~> 1.61) + rubocop-openproject (0.1.0) + rubocop rubocop-performance (1.21.1) rubocop (>= 1.48.1, < 2.0) rubocop-ast (>= 1.31.1, < 2.0) @@ -1320,7 +1314,7 @@ DEPENDENCIES rubocop rubocop-capybara rubocop-factory_bot - rubocop-openproject! + rubocop-openproject rubocop-performance rubocop-rails rubocop-rspec