Skip to content
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

AO3-6691 Custom cops for house rules #4763

Merged
merged 12 commits into from
Apr 2, 2024
50 changes: 50 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
require:
- rubocop-rails
- rubocop-rspec
- ./rubocop/rubocop

inherit_mode:
merge:
Expand All @@ -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

Expand Down Expand Up @@ -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
Expand Down
51 changes: 51 additions & 0 deletions rubocop/cop/cucumber/regex_step_name.rb
Original file line number Diff line number Diff line change
@@ -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.
Comment on lines +7 to +8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that the cop will have false failures in those cases, leading to false positive failing tests on PRs. That's not great. Perhaps that would be helped if the offense could be considered a low severity. Though I don't know if that will keep the test from failing while still showing up on GitHub, I'd have to investigate how reviewdog handles it.

But did find that add_offense indeed supports a severity level.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what it looks like with severity: :convention https://github.com/brianjaustin/ao3-sandbox/pull/9/files

The action is still marked as failed, but at least the annotation looks a little bit less angry (the higher severities are either yellow or red with an ❌)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As tested here that unfortunately doesn't apply for commits from forks where reviewdog degrades to annotations. Oh well, was worth the try.

#
# @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"

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
34 changes: 34 additions & 0 deletions rubocop/cop/i18n/default_translation.rb
Original file line number Diff line number Diff line change
@@ -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
31 changes: 31 additions & 0 deletions rubocop/cop/i18n/deprecated_helper.rb
Original file line number Diff line number Diff line change
@@ -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
47 changes: 47 additions & 0 deletions rubocop/cop/i18n/deprecated_translation_key.rb
Original file line number Diff line number Diff line change
@@ -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
61 changes: 61 additions & 0 deletions rubocop/cop/migration/large_table_schema_update.rb
Original file line number Diff line number Diff line change
@@ -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
5 changes: 5 additions & 0 deletions rubocop/rubocop.rb
Original file line number Diff line number Diff line change
@@ -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 }
69 changes: 69 additions & 0 deletions spec/rubocop/cop/cucumber/regex_step_name_spec.rb
Original file line number Diff line number Diff line change
@@ -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
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
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
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
Loading
Loading