From 181cd299f9e06223e8338e93b1c318c671ccb1aa Mon Sep 17 00:00:00 2001 From: Jacopo Date: Tue, 14 Nov 2017 10:02:39 +0100 Subject: Adds Rubocop rule for line break after guard clause Adds a rubocop rule (with autocorrect) to ensure line break after guard clauses. --- rubocop/cop/line_break_after_guard_clauses.rb | 100 ++++++++++++++++++++++++++ rubocop/rubocop.rb | 1 + 2 files changed, 101 insertions(+) create mode 100644 rubocop/cop/line_break_after_guard_clauses.rb (limited to 'rubocop') diff --git a/rubocop/cop/line_break_after_guard_clauses.rb b/rubocop/cop/line_break_after_guard_clauses.rb new file mode 100644 index 00000000000..67477f064ab --- /dev/null +++ b/rubocop/cop/line_break_after_guard_clauses.rb @@ -0,0 +1,100 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Ensures a line break after guard clauses. + # + # @example + # # bad + # return unless condition + # do_stuff + # + # # good + # return unless condition + # + # do_stuff + # + # # bad + # raise if condition + # do_stuff + # + # # good + # raise if condition + # + # do_stuff + # + # Multiple guard clauses are allowed without + # line break. + # + # # good + # return unless condition_a + # return unless condition_b + # + # do_stuff + # + # Guard clauses in case statement are allowed without + # line break. + # + # # good + # case model + # when condition_a + # return true unless condition_b + # when + # ... + # end + # + # Guard clauses before end are allowed without + # line break. + # + # # good + # if condition_a + # do_something + # else + # do_something_else + # return unless condition + # end + # + # do_something_more + class LineBreakAfterGuardClauses < RuboCop::Cop::Cop + MSG = 'Add a line break after guard clauses' + + def_node_matcher :guard_clause_node?, <<-PATTERN + [{(send nil? {:raise :fail :throw} ...) return break next} single_line?] + PATTERN + + def on_if(node) + return unless node.single_line? + return unless guard_clause?(node) + return if next_line(node).blank? || clause_last_line?(next_line(node)) || guard_clause?(next_sibling(node)) + + add_offense(node, :expression, MSG) + end + + def autocorrect(node) + lambda do |corrector| + corrector.insert_after(node.loc.expression, "\n") + end + end + + private + + def guard_clause?(node) + return false unless node.if_type? + + guard_clause_node?(node.if_branch) + end + + def next_sibling(node) + node.parent.children[node.sibling_index + 1] + end + + def next_line(node) + processed_source[node.loc.line] + end + + def clause_last_line?(line) + line =~ /^\s*(?:end|elsif|else|when|rescue|ensure)/ + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 4ebbe010e90..0992168e1ff 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -3,6 +3,7 @@ require_relative 'cop/active_record_serialize' require_relative 'cop/custom_error_class' require_relative 'cop/gem_fetcher' require_relative 'cop/in_batches' +require_relative 'cop/line_break_after_guard_clauses' require_relative 'cop/polymorphic_associations' require_relative 'cop/project_path_helper' require_relative 'cop/redirect_with_status' -- cgit v1.2.3 From d8be981466e2a794b42996960f1fdb91e560a707 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 17 Nov 2017 13:38:14 +0000 Subject: Prevent update_column_in_batches on large tables add_column_with_default is implemented in terms of update_column_in_batches, but update_column_in_batches can be used independently. Neither of these should be used on the specified large tables, because they will cause issues on large instances like GitLab.com. This also ignores the cop for all existing migrations, renaming AddColumnWithDefaultToLargeTable where appropriate. --- .../add_column_with_default_to_large_table.rb | 55 --------------------- rubocop/cop/migration/update_large_table.rb | 57 ++++++++++++++++++++++ rubocop/rubocop.rb | 2 +- 3 files changed, 58 insertions(+), 56 deletions(-) delete mode 100644 rubocop/cop/migration/add_column_with_default_to_large_table.rb create mode 100644 rubocop/cop/migration/update_large_table.rb (limited to 'rubocop') diff --git a/rubocop/cop/migration/add_column_with_default_to_large_table.rb b/rubocop/cop/migration/add_column_with_default_to_large_table.rb deleted file mode 100644 index fb363f95b56..00000000000 --- a/rubocop/cop/migration/add_column_with_default_to_large_table.rb +++ /dev/null @@ -1,55 +0,0 @@ -require_relative '../../migration_helpers' - -module RuboCop - module Cop - module Migration - # This cop checks for `add_column_with_default` on a table that's been - # explicitly blacklisted because of its size. - # - # Even though this helper performs the update in batches to avoid - # downtime, using it with tables with millions of rows still causes a - # significant delay in the deploy process and is best avoided. - # - # See https://gitlab.com/gitlab-com/infrastructure/issues/1602 for more - # information. - class AddColumnWithDefaultToLargeTable < RuboCop::Cop::Cop - include MigrationHelpers - - MSG = 'Using `add_column_with_default` on the `%s` table will take a ' \ - 'long time to complete, and should be avoided unless absolutely ' \ - 'necessary'.freeze - - LARGE_TABLES = %i[ - ci_pipelines - ci_builds - events - issues - merge_request_diff_files - merge_request_diffs - merge_requests - namespaces - notes - projects - routes - users - ].freeze - - def_node_matcher :add_column_with_default?, <<~PATTERN - (send nil :add_column_with_default $(sym ...) ...) - PATTERN - - def on_send(node) - return unless in_migration?(node) - - matched = add_column_with_default?(node) - return unless matched - - table = matched.to_a.first - return unless LARGE_TABLES.include?(table) - - add_offense(node, :expression, format(MSG, table)) - end - end - end - end -end diff --git a/rubocop/cop/migration/update_large_table.rb b/rubocop/cop/migration/update_large_table.rb new file mode 100644 index 00000000000..3ae3fb1b68e --- /dev/null +++ b/rubocop/cop/migration/update_large_table.rb @@ -0,0 +1,57 @@ +require_relative '../../migration_helpers' + +module RuboCop + module Cop + module Migration + # This cop checks for `add_column_with_default` on a table that's been + # explicitly blacklisted because of its size. + # + # Even though this helper performs the update in batches to avoid + # downtime, using it with tables with millions of rows still causes a + # significant delay in the deploy process and is best avoided. + # + # See https://gitlab.com/gitlab-com/infrastructure/issues/1602 for more + # information. + class UpdateLargeTable < RuboCop::Cop::Cop + include MigrationHelpers + + MSG = 'Using `%s` on the `%s` table will take a long time to ' \ + 'complete, and should be avoided unless absolutely ' \ + 'necessary'.freeze + + LARGE_TABLES = %i[ + ci_pipelines + ci_builds + events + issues + merge_request_diff_files + merge_request_diffs + merge_requests + namespaces + notes + projects + routes + users + ].freeze + + def_node_matcher :batch_update?, <<~PATTERN + (send nil ${:add_column_with_default :update_column_in_batches} $(sym ...) ...) + PATTERN + + def on_send(node) + return unless in_migration?(node) + + matches = batch_update?(node) + return unless matches + + update_method = matches.first + table = matches.last.to_a.first + + return unless LARGE_TABLES.include?(table) + + add_offense(node, :expression, format(MSG, update_method, table)) + end + end + end + end +end diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb index 4ebbe010e90..a1668749dd9 100644 --- a/rubocop/rubocop.rb +++ b/rubocop/rubocop.rb @@ -7,7 +7,6 @@ require_relative 'cop/polymorphic_associations' require_relative 'cop/project_path_helper' require_relative 'cop/redirect_with_status' require_relative 'cop/migration/add_column' -require_relative 'cop/migration/add_column_with_default_to_large_table' require_relative 'cop/migration/add_concurrent_foreign_key' require_relative 'cop/migration/add_concurrent_index' require_relative 'cop/migration/add_index' @@ -20,6 +19,7 @@ require_relative 'cop/migration/reversible_add_column_with_default' require_relative 'cop/migration/safer_boolean_column' require_relative 'cop/migration/timestamps' require_relative 'cop/migration/update_column_in_batches' +require_relative 'cop/migration/update_large_table' require_relative 'cop/rspec/env_assignment' require_relative 'cop/rspec/single_line_hook' require_relative 'cop/rspec/verbose_include_metadata' -- cgit v1.2.3