From cedf5b6dd8c4183f74bce7e3eaf5d280e045d243 Mon Sep 17 00:00:00 2001 From: Brian Austin <13002992+brianjaustin@users.noreply.github.com> Date: Tue, 2 Apr 2024 02:09:14 -0400 Subject: [PATCH] AO3-6691 Custom cops for house rules (#4763) * Helpers for custom cops * Cop: large table migrations * Cop: regex cucumber steps * Cop: guard against `ts` * Cop: guard against name_with_colon * Cop: prevent default scope * Link to i18n wiki * Extract config to YAML * Cop: default kwarg in t helper * Update severities * Cleanup last I18n.t -> t comments * link to rubocop suppress docs --- .rubocop.yml | 50 ++++++++++++++ rubocop/cop/cucumber/regex_step_name.rb | 51 ++++++++++++++ rubocop/cop/i18n/default_translation.rb | 34 +++++++++ rubocop/cop/i18n/deprecated_helper.rb | 31 +++++++++ .../cop/i18n/deprecated_translation_key.rb | 47 +++++++++++++ .../migration/large_table_schema_update.rb | 61 ++++++++++++++++ rubocop/rubocop.rb | 5 ++ .../cop/cucumber/regex_step_name_spec.rb | 69 +++++++++++++++++++ .../cop/i18n/default_translation_spec.rb | 37 ++++++++++ .../cop/i18n/deprecated_helper_spec.rb | 33 +++++++++ .../i18n/deprecated_translation_key_spec.rb | 34 +++++++++ .../large_table_schema_update_spec.rb | 53 ++++++++++++++ spec/rubocop_spec_helper.rb | 20 ++++++ 13 files changed, 525 insertions(+) create mode 100644 rubocop/cop/cucumber/regex_step_name.rb create mode 100644 rubocop/cop/i18n/default_translation.rb create mode 100644 rubocop/cop/i18n/deprecated_helper.rb create mode 100644 rubocop/cop/i18n/deprecated_translation_key.rb create mode 100644 rubocop/cop/migration/large_table_schema_update.rb create mode 100644 rubocop/rubocop.rb create mode 100644 spec/rubocop/cop/cucumber/regex_step_name_spec.rb create mode 100644 spec/rubocop/cop/i18n/default_translation_spec.rb create mode 100644 spec/rubocop/cop/i18n/deprecated_helper_spec.rb create mode 100644 spec/rubocop/cop/i18n/deprecated_translation_key_spec.rb create mode 100644 spec/rubocop/cop/migration/large_table_schema_update_spec.rb create mode 100644 spec/rubocop_spec_helper.rb diff --git a/.rubocop.yml b/.rubocop.yml index d6fdc3dd71b..6ca41445023 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -6,6 +6,7 @@ require: - rubocop-rails - rubocop-rspec + - ./rubocop/rubocop inherit_mode: merge: @@ -18,6 +19,10 @@ AllCops: Bundler/OrderedGems: Enabled: false +I18n/DeprecatedTranslationKey: + Rules: + name_with_colon: "Prefer `name` with `mailer.general.metadata_label_indicator` over `name_with_colon`" + Layout/DotPosition: EnforcedStyle: leading @@ -72,6 +77,51 @@ Metrics/ParameterLists: Metrics/PerceivedComplexity: Enabled: false +Migration/LargeTableSchemaUpdate: + Tables: + - abuse_reports + - admin_activities + - audits + - bookmarks + - comments + - common_taggings + - collection_items + - collection_participants + - collection_profiles + - collections + - creatorships + - external_works + - favorite_tags + - feedbacks + - filter_counts + - filter_taggings + - gifts + - inbox_comments + - invitations + - kudos + - log_items + - mutes + - preferences + - profiles + - prompts + - pseuds + - readings + - set_taggings + - serial_works + - series + - skins + - stat_counters + - subscriptions + - tag_nominations + - tag_set_associations + - tags + - taggings + - users + - works + +Rails/DefaultScope: + Enabled: true + Rails/DynamicFindBy: AllowedMethods: # Exception for Tag.find_by_name diff --git a/rubocop/cop/cucumber/regex_step_name.rb b/rubocop/cop/cucumber/regex_step_name.rb new file mode 100644 index 00000000000..2c354ba267b --- /dev/null +++ b/rubocop/cop/cucumber/regex_step_name.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Cucumber + # Checks that Cucumber step definitions use Cucumber expressions + # instead of Regex. Note: this may not always be possible, and this + # cop is not smart enough to detect those cases. + # + # @example + # # bad + # Given /foobar/ do + # ... + # end + # When /baz/ do + # ... + # end + # Then /oops(\w+)/ do |it| + # ... + # end + # + # @example + # # good + # Given "foobar(s)" do + # ... + # end + # When "baz" do + # ... + # end + # Then "oops{str}" do |it| + # ... + # end + class RegexStepName < RuboCop::Cop::Base + MSG = "Prefer Cucumber expressions (https://github.com/cucumber/cucumber-expressions) over regex for step names; refer to https://github.com/otwcode/otwarchive/wiki/Reviewdog-and-RuboCop if regex is still required" + + RESTRICT_ON_SEND = %i[Given When Then].freeze + + # @!method regex_name(node) + def_node_matcher :regex_name, <<~PATTERN + (send nil? _ $(:regexp ...) ...) + PATTERN + + def on_send(node) + regex_name(node) do |regex_node| + add_offense(regex_node, severity: :refactor) + end + end + end + end + end +end diff --git a/rubocop/cop/i18n/default_translation.rb b/rubocop/cop/i18n/default_translation.rb new file mode 100644 index 00000000000..c59839b7a9f --- /dev/null +++ b/rubocop/cop/i18n/default_translation.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module I18n + # Checks for uses of the `default` keyword argument within Rails translation helpers. + # + # @example + # # bad + # t(".translation_key", default: "English text") + # + # @example + # # good + # # assuming the translation is in a view, the key must be defined in config/locales/views/en.yml + # t(".translation_key") + class DefaultTranslation < RuboCop::Cop::Base + MSG = "Prefer setting a translation in the appropriate `en.yml` locale file instead of using `default`" + + RESTRICT_ON_SEND = %i[t translate].freeze + + # @!method default_kwarg(node) + def_node_search :default_kwarg, <<~PATTERN + (pair (sym :default) _) + PATTERN + + def on_send(node) + default_kwarg(node).each do |kwarg_node| + add_offense(kwarg_node) + end + end + end + end + end +end diff --git a/rubocop/cop/i18n/deprecated_helper.rb b/rubocop/cop/i18n/deprecated_helper.rb new file mode 100644 index 00000000000..8f23b57b01d --- /dev/null +++ b/rubocop/cop/i18n/deprecated_helper.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module I18n + # Checks for uses of the deprecated helper function, `ts`. + # Strings passed to it cannot be translated, and all calls + # will need to be replaced with `t` to enable UI translations + # in the future. + # + # @example + # # bad + # ts("This will only be in English :(") + # ts("Hello %{name}", name: "world") + # + # @example + # # good + # t(".relative.path.to.translation") + # t(".greeting", name: "world") + class DeprecatedHelper < RuboCop::Cop::Base + MSG = "Prefer Rails built-in `t` helper over `ts`: the latter is not actually translatable. For more information, refer to https://github.com/otwcode/otwarchive/wiki/Internationalization-(i18n)-Standards." + + RESTRICT_ON_SEND = %i[ts].freeze + + def on_send(node) + add_offense(node) + end + end + end + end +end diff --git a/rubocop/cop/i18n/deprecated_translation_key.rb b/rubocop/cop/i18n/deprecated_translation_key.rb new file mode 100644 index 00000000000..5f30b623fcd --- /dev/null +++ b/rubocop/cop/i18n/deprecated_translation_key.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module I18n + # Checks for uses of translation keys that have been superseded + # by others or different methods of translation. + # + # @example + # # bad + # Category.human_attribute_name("name_with_colon", count: 1) + # t(".relative.path.name_with_colon", count: 2) + # + # @example + # # good + # Category.human_attribute_name("name", count: 1) + t("mailer.general.metadata_label_indicator") + # metadata_property(t(".relative.path.name", count: 2)) # views only + class DeprecatedTranslationKey < RuboCop::Cop::Base + # Rubocop optimization: the check here is a little bit inefficient, + # and we know which functions/methods to check, so only run it on those. + RESTRICT_ON_SEND = %i[t translate human_attribute_name].freeze + + # @!method translation_keys(node) + def_node_search :translation_keys, <<~PATTERN + $(str $_) + PATTERN + + def on_send(node) + translation_keys(node).each do |key_node, key_text| + denied_key = deprecated_keys.find do |key, _| + key_text.include?(key.to_s) + end + next unless denied_key + + add_offense(key_node, message: denied_key[1]) + end + end + + private + + def deprecated_keys + cop_config["Rules"] || [] + end + end + end + end +end diff --git a/rubocop/cop/migration/large_table_schema_update.rb b/rubocop/cop/migration/large_table_schema_update.rb new file mode 100644 index 00000000000..1061b9293fe --- /dev/null +++ b/rubocop/cop/migration/large_table_schema_update.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Migration + # Checks that migrations updating the schema of large tables, + # as defined in the configuration, do so safely. As of writing, + # this involves utilizing the `uses_departure!` helper. + # + # @example + # # good + # class ExampleMigration < ActiveRecord::Migration[6.1] + # uses_departure! if Rails.env.staging? || Rails.env.production? + # + # add_column :users, :new_field, :integer, nullable: true + # end + # + # @example + # # bad + # class ExampleMigration < ActiveRecord::Migration[6.1] + # add_column :users, :new_field, :integer, nullable: true + # end + class LargeTableSchemaUpdate < RuboCop::Cop::Base + MSG = "Use departure to change the schema of large table `%s`" + + # @!method uses_departure?(node) + def_node_search :uses_departure?, <<~PATTERN + (send nil? :uses_departure!) + PATTERN + + # @!method schema_changes(node) + def_node_search :schema_changes, <<~PATTERN + $(send nil? _ (sym $_) ...) + PATTERN + + def on_class(node) + return unless in_migration?(node) + return if uses_departure?(node) + + schema_changes(node).each do |change_node, table_name| + next unless large_tables.include?(table_name.to_s) + + add_offense(change_node.loc.expression, message: format(MSG, table_name), severity: :warning) + end + end + + private + + def large_tables + cop_config["Tables"] || [] + end + + def in_migration?(node) + filename = node.location.expression.source_buffer.name + directory = File.dirname(filename) + directory.end_with?("db/migrate") + end + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb new file mode 100644 index 00000000000..63c2b902fa8 --- /dev/null +++ b/rubocop/rubocop.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +# This comes from GitLab's Rubocop setup +# Auto-require all cops under `rubocop/cop/**/*.rb` +Dir[File.join(__dir__, "cop", "**", "*.rb")].each { |file| require file } diff --git a/spec/rubocop/cop/cucumber/regex_step_name_spec.rb b/spec/rubocop/cop/cucumber/regex_step_name_spec.rb new file mode 100644 index 00000000000..4e5d2e5e5bd --- /dev/null +++ b/spec/rubocop/cop/cucumber/regex_step_name_spec.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +require "rubocop_spec_helper" +require_relative "../../../../rubocop/cop/cucumber/regex_step_name" + +describe RuboCop::Cop::Cucumber::RegexStepName do + context "with a `Given` block" do + it "records a violation when named via regex" do + expect_offense(<<~INVALID) + Given /^I have no users$/ do + ^^^^^^^^^^^^^^^^^^^ Prefer Cucumber expressions (https://github.com/cucumber/cucumber-expressions) over regex for step names; refer to https://github.com/otwcode/otwarchive/wiki/Reviewdog-and-RuboCop if regex is still required + User.delete_all + end + INVALID + end + + it "does not record a violation when named via a Cucumber expression" do + expect_no_offenses(<<~RUBY) + Given "I have no users" do + User.delete_all + end + RUBY + end + end + + context "with a `When` block" do + it "records a violation when named via regex" do + expect_offense(<<~INVALID) + When /^I visit the change username page for (.*)$/ do |login| + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer Cucumber expressions (https://github.com/cucumber/cucumber-expressions) over regex for step names; refer to https://github.com/otwcode/otwarchive/wiki/Reviewdog-and-RuboCop if regex is still required + user = User.find_by(login: login) + visit change_username_user_path(user) + end + INVALID + end + + it "does not record a violation when named via a Cucumber expression" do + expect_no_offenses(<<~RUBY) + When "I request a password reset for {string}" do |login| + step(%{I am on the login page}) + step(%{I follow "Reset password"}) + step(%{I fill in "Email address or user name" with "\#{login}"}) + step(%{I press "Reset Password"}) + end + RUBY + end + end + + context "with a `Then` block" do + it "records a violation when named via regex" do + expect_offense(<<~INVALID) + Then /^the user "([^"]*)" should be activated$/ do |login| + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer Cucumber expressions (https://github.com/cucumber/cucumber-expressions) over regex for step names; refer to https://github.com/otwcode/otwarchive/wiki/Reviewdog-and-RuboCop if regex is still required + user = User.find_by(login: login) + expect(user).to be_active + end + INVALID + end + + it "does not record a violation when named via a Cucumber expression" do + expect_no_offenses(<<~RUBY) + Then "I should see the invitation id for the user {string}" do |login| + invitation_id = User.find_by(login: login).invitation.id + step %{I should see "Invitation: \#{invitation_id}"} + end + RUBY + end + end +end diff --git a/spec/rubocop/cop/i18n/default_translation_spec.rb b/spec/rubocop/cop/i18n/default_translation_spec.rb new file mode 100644 index 00000000000..6111fbbd558 --- /dev/null +++ b/spec/rubocop/cop/i18n/default_translation_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require "rubocop_spec_helper" +require_relative "../../../../rubocop/cop/i18n/default_translation" + +describe RuboCop::Cop::I18n::DefaultTranslation do + context "when within the `t` helper" do + it "registers an offense if `default` is used alone" do + expect_offense(<<~INVALID) + t(".translation_key", default: "English text") + ^^^^^^^^^^^^^^^^^^^^^^^ Prefer setting a translation in the appropriate `en.yml` locale file instead of using `default` + INVALID + end + + it "registers an offense if `default` is used with other kwargs" do + expect_offense(<<~INVALID) + t(".translation_key", input: "hello", default: "I got %{input}") + ^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer setting a translation in the appropriate `en.yml` locale file instead of using `default` + INVALID + end + + it "does not register an offense if `default` is not used" do + expect_no_offenses(<<~RUBY) + t(".translation_key1") + t(".translation_key2", input: "hello") + RUBY + end + end + + context "when not within the `t` helper" do + it "does not register an offense if `default` is present in keyword args" do + expect_no_offenses(<<~RUBY) + my_method("arg", default: "something", kwarg1: "hi") + RUBY + end + end +end diff --git a/spec/rubocop/cop/i18n/deprecated_helper_spec.rb b/spec/rubocop/cop/i18n/deprecated_helper_spec.rb new file mode 100644 index 00000000000..c8741f5807b --- /dev/null +++ b/spec/rubocop/cop/i18n/deprecated_helper_spec.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +require "rubocop_spec_helper" +require_relative "../../../../rubocop/cop/i18n/deprecated_helper" + +describe RuboCop::Cop::I18n::DeprecatedHelper do + it "registers an offense when `ts` is used" do + expect_offense(<<~INVALID) + ts("Some String") + ^^^^^^^^^^^^^^^^^ Prefer Rails built-in `t` helper over `ts`: the latter is not actually translatable. For more information, refer to https://github.com/otwcode/otwarchive/wiki/Internationalization-(i18n)-Standards. + INVALID + end + + it "registers an offense when `ts` is used without parentheses" do + expect_offense(<<~INVALID) + ts "Another string" + ^^^^^^^^^^^^^^^^^^^ Prefer Rails built-in `t` helper over `ts`: the latter is not actually translatable. For more information, refer to https://github.com/otwcode/otwarchive/wiki/Internationalization-(i18n)-Standards. + INVALID + end + + it "does not register an offense when `I18n.t` is used" do + expect_no_offenses(<<~RUBY) + I18n.t(".hello") + t(".goodbye") + RUBY + end + + it "does not register an offense for functions containing the letter `ts`" do + expect_no_offenses(<<~RUBY) + cats("meow") + RUBY + end +end diff --git a/spec/rubocop/cop/i18n/deprecated_translation_key_spec.rb b/spec/rubocop/cop/i18n/deprecated_translation_key_spec.rb new file mode 100644 index 00000000000..bb73fe625ee --- /dev/null +++ b/spec/rubocop/cop/i18n/deprecated_translation_key_spec.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +require "rubocop_spec_helper" +require_relative "../../../../rubocop/cop/i18n/deprecated_translation_key" + +describe RuboCop::Cop::I18n::DeprecatedTranslationKey do + subject(:cop) { described_class.new(config) } + + let(:config) do + RuboCop::Config.new("I18n/DeprecatedTranslationKey" => { + "Rules" => { + "name_with_colon" => "Prefer `name` with `mailer.general.metadata_label_indicator` over `name_with_colon`" + } + }) + end + + context "when using I18n.translate" do + it "records a violation for `name_with_colon`" do + expect_offense(<<~INVALID) + I18n.translate("name_with_colon") + ^^^^^^^^^^^^^^^^^ Prefer `name` with `mailer.general.metadata_label_indicator` over `name_with_colon` + INVALID + end + end + + context "when using human_attribute_name" do + it "records a violation for `name_with_colon`" do + expect_offense(<<~INVALID) + Fandom.human_attribute_name("name_with_colon", count: prompt.any_fandom ? 1 : tag_groups["Fandom"].count) + ^^^^^^^^^^^^^^^^^ Prefer `name` with `mailer.general.metadata_label_indicator` over `name_with_colon` + INVALID + end + end +end diff --git a/spec/rubocop/cop/migration/large_table_schema_update_spec.rb b/spec/rubocop/cop/migration/large_table_schema_update_spec.rb new file mode 100644 index 00000000000..23d876f4faa --- /dev/null +++ b/spec/rubocop/cop/migration/large_table_schema_update_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require "rubocop_spec_helper" +require_relative "../../../../rubocop/cop/migration/large_table_schema_update" + +describe RuboCop::Cop::Migration::LargeTableSchemaUpdate do + subject(:cop) { described_class.new(config) } + + let(:config) { RuboCop::Config.new("Migration/LargeTableSchemaUpdate" => { "Tables" => ["users"] }) } + + context "when running on a migration file" do + before do + allow(cop).to receive(:in_migration?).and_return(true) + end + + context "when modifying a large table" do + it "registers an offense if Departure is not used" do + expect_offense(<<~RUBY) + class FakeMigration < ActiveRecord::Migration[6.1] + def change + add_column :users, :foo, :boolean, default: false, null: false + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use departure to change the schema of large table `users` + end + end + RUBY + end + + it "does not register an offense if Departure is used" do + expect_no_offenses(<<~RUBY) + class FakeMigration < ActiveRecord::Migration[6.1] + uses_departure! if Rails.env.staging? || Rails.env.production? + + def change + add_column :users, :foo, :boolean, default: false, null: false + end + end + RUBY + end + end + + context "when modifying a small table" do + it "does not register an offense if Departure is not used" do + expect_no_offenses(<<~RUBY) + class FakeMigration < ActiveRecord::Migration[6.1] + def change + add_column :small, :foo, :boolean, default: false, null: false + end + end + RUBY + end + end + end +end diff --git a/spec/rubocop_spec_helper.rb b/spec/rubocop_spec_helper.rb new file mode 100644 index 00000000000..e5997cafba6 --- /dev/null +++ b/spec/rubocop_spec_helper.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +require "spec_helper" +require "rubocop" +require "rubocop/rspec/cop_helper" +require "rubocop/rspec/expect_offense" +require "rubocop/rspec/host_environment_simulation_helper" +require "rubocop/rspec/shared_contexts" + +RSpec.configure do |config| + config.define_derived_metadata(file_path: %r{spec/rubocop}) do |metadata| + metadata[:type] = :rubocop + end + + config.include CopHelper, type: :rubocop + config.include HostEnvironmentSimulatorHelper, type: :rubocop + config.include RuboCop::RSpec::ExpectOffense, type: :rubocop + + config.include_context "config", type: :rubocop +end