From 563bcfbefd81475cb75e627ebd6b26d4fdacacfb Mon Sep 17 00:00:00 2001 From: Mantas Masalskis Date: Tue, 18 Aug 2020 15:01:18 +0200 Subject: [PATCH] Maintenance: Custom Rubocop matcher for making sure "unless" conditional is not used in Ruby --- .../zammad/prefer_negated_if_over_unless.rb | 30 +++++++++++++++++++ .rubocop/default.yml | 4 +++ .rubocop/rubocop_zammad.rb | 1 + app/helpers/knowledge_base_helper.rb | 14 ++++----- .../knowledge_base_rich_text_helper.rb | 2 +- app/models/channel/driver/imap.rb | 2 +- app/models/knowledge_base.rb | 5 ++++ lib/knowledge_base/menu_item_update_action.rb | 2 +- lib/tasks/zammad/setup/db_config.rake | 2 +- script/scheduler.rb | 2 +- script/websocket-server.rb | 2 +- spec/support/db_migration.rb | 2 +- spec/system/ticket/zoom_spec.rb | 2 +- test/browser_test_helper.rb | 2 +- 14 files changed, 54 insertions(+), 18 deletions(-) create mode 100644 .rubocop/cop/zammad/prefer_negated_if_over_unless.rb diff --git a/.rubocop/cop/zammad/prefer_negated_if_over_unless.rb b/.rubocop/cop/zammad/prefer_negated_if_over_unless.rb new file mode 100644 index 000000000..49b0d8a9b --- /dev/null +++ b/.rubocop/cop/zammad/prefer_negated_if_over_unless.rb @@ -0,0 +1,30 @@ +module RuboCop + module Cop + module Zammad + # This cop is used to identify usages of `unless` conditionals + # + # @example + # # bad + # unless statement + # return unless statement + # + # # good + # if !statement + # return if !statement + class PreferNegatedIfOverUnless < Cop + include ConfigurableEnforcedStyle + include NegativeConditional + extend AutoCorrector + + MSG = 'Favor `if !foobar` over `unless foobar` for ' \ + 'negative conditions.'.freeze + + def on_if(node) + return unless node.unless? + + add_offense(node, message: MSG) + end + end + end + end +end diff --git a/.rubocop/default.yml b/.rubocop/default.yml index 476aa0def..6b0946d3e 100644 --- a/.rubocop/default.yml +++ b/.rubocop/default.yml @@ -291,3 +291,7 @@ Layout/MultilineMethodCallIndentation: EnforcedStyle: indented Include: - "**/*_spec.rb" + +Zammad/PreferNegatedIfOverUnless: + Exclude: + - 'bin/rspec' diff --git a/.rubocop/rubocop_zammad.rb b/.rubocop/rubocop_zammad.rb index d34a735a7..552e9f0b5 100644 --- a/.rubocop/rubocop_zammad.rb +++ b/.rubocop/rubocop_zammad.rb @@ -1 +1,2 @@ require_relative 'cop/zammad/exists_condition' +require_relative 'cop/zammad/prefer_negated_if_over_unless' diff --git a/app/helpers/knowledge_base_helper.rb b/app/helpers/knowledge_base_helper.rb index f7403ab28..5b71afb36 100644 --- a/app/helpers/knowledge_base_helper.rb +++ b/app/helpers/knowledge_base_helper.rb @@ -6,19 +6,15 @@ module KnowledgeBaseHelper end def custom_path_if_needed(path, knowledge_base, full: false) - return path unless knowledge_base.custom_address_matches? request + return path if !knowledge_base.custom_address_matches?(request) custom_address = knowledge_base.custom_address_uri - return path unless custom_address + return path if !custom_address - output = path.gsub(%r{^/help}, custom_address.path || '').presence || '/' + custom_path = path.gsub(%r{^/help}, custom_address.path || '').presence || '/' + prefix = full ? knowledge_base.custom_path_prefix(request) : '' - if full - fqdn = request.headers.env['SERVER_NAME'] - output = "#{custom_address.scheme}://#{custom_address.host || fqdn}#{output}" - end - - output + "#{prefix}#{custom_path}" end def translation_locale_code(translation) diff --git a/app/helpers/knowledge_base_rich_text_helper.rb b/app/helpers/knowledge_base_rich_text_helper.rb index 46bcb15aa..6aa8c8418 100644 --- a/app/helpers/knowledge_base_rich_text_helper.rb +++ b/app/helpers/knowledge_base_rich_text_helper.rb @@ -42,7 +42,7 @@ module KnowledgeBaseRichTextHelper "https://player.vimeo.com/video/#{settings[:id]}" end - return match unless url + return match if !url "
" end diff --git a/app/models/channel/driver/imap.rb b/app/models/channel/driver/imap.rb index 090dd9ee5..b9a3886f7 100644 --- a/app/models/channel/driver/imap.rb +++ b/app/models/channel/driver/imap.rb @@ -378,7 +378,7 @@ returns def self.extract_rfc822_headers(message_meta) blob = message_meta&.attr&.dig 'RFC822.HEADER' - return unless blob + return if !blob parse_rfc822_headers blob end diff --git a/app/models/knowledge_base.rb b/app/models/knowledge_base.rb index 6bf240592..909575b69 100644 --- a/app/models/knowledge_base.rb +++ b/app/models/knowledge_base.rb @@ -93,6 +93,11 @@ class KnowledgeBase < ApplicationModel false end + def custom_address_prefix(request) + host = custom_address.host || request.headers.env['SERVER_NAME'] + "#{custom_address.scheme}://#{host}" + end + def full_destroy! ChecksKbClientNotification.disable_in_all_classes! diff --git a/lib/knowledge_base/menu_item_update_action.rb b/lib/knowledge_base/menu_item_update_action.rb index a85ddf059..902aff9b1 100644 --- a/lib/knowledge_base/menu_item_update_action.rb +++ b/lib/knowledge_base/menu_item_update_action.rb @@ -11,7 +11,7 @@ class KnowledgeBase end def perform! - raise_unprocessable unless all_ids_present? + raise_unprocessable if !all_ids_present? KnowledgeBase::MenuItem.transaction do KnowledgeBase::MenuItem.acts_as_list_no_update do diff --git a/lib/tasks/zammad/setup/db_config.rake b/lib/tasks/zammad/setup/db_config.rake index a499a052c..4f27bd69d 100644 --- a/lib/tasks/zammad/setup/db_config.rake +++ b/lib/tasks/zammad/setup/db_config.rake @@ -9,7 +9,7 @@ namespace :zammad do template = config_dir.join('database', 'database.yml') destination = config_dir.join('database.yml') - raise Errno::ENOENT, "#{template} not found" unless File.exist?(template) + raise Errno::ENOENT, "#{template} not found" if !File.exist?(template) if File.exist?(destination) next if FileUtils.identical?(template, destination) diff --git a/script/scheduler.rb b/script/scheduler.rb index c85b15615..47ad3da65 100755 --- a/script/scheduler.rb +++ b/script/scheduler.rb @@ -3,7 +3,7 @@ begin load File.expand_path('../bin/spring', __dir__) rescue LoadError => e - raise unless e.message.include?('spring') + raise if !e.message.include?('spring') end dir = File.expand_path(File.join(File.dirname(__FILE__), '..')) diff --git a/script/websocket-server.rb b/script/websocket-server.rb index a59885bbf..f850f2a1c 100755 --- a/script/websocket-server.rb +++ b/script/websocket-server.rb @@ -3,7 +3,7 @@ begin load File.expand_path('../bin/spring', __dir__) rescue LoadError => e - raise unless e.message.include?('spring') + raise if !e.message.include?('spring') end dir = File.expand_path(File.join(File.dirname(__FILE__), '..')) diff --git a/spec/support/db_migration.rb b/spec/support/db_migration.rb index beb8a407f..9640e6a2c 100644 --- a/spec/support/db_migration.rb +++ b/spec/support/db_migration.rb @@ -67,7 +67,7 @@ module DbMigrationHelper def without_column(from_table, column:) suppress_messages do Array(column).each do |elem| - next unless column_exists?(from_table, elem) + next if !column_exists?(from_table, elem) remove_column(from_table, elem) end diff --git a/spec/system/ticket/zoom_spec.rb b/spec/system/ticket/zoom_spec.rb index d237e644e..81861b735 100644 --- a/spec/system/ticket/zoom_spec.rb +++ b/spec/system/ticket/zoom_spec.rb @@ -859,7 +859,7 @@ RSpec.describe 'Ticket zoom', type: :system do image_a.height.times do |y| image_a.row(y).each_with_index do |pixel, x| - return false unless pixel == image_b[x, y] + return false if pixel != image_b[x, y] end end diff --git a/test/browser_test_helper.rb b/test/browser_test_helper.rb index c221cd665..50b224d93 100644 --- a/test/browser_test_helper.rb +++ b/test/browser_test_helper.rb @@ -514,7 +514,7 @@ class TestCase < ActiveSupport::TestCase begin elements = instance.find_elements(find_element_key => params[param_key]) - .tap { |e| e.slice!(1..-1) unless params[:all] } + .tap { |e| e.slice!(1..-1) if !params[:all] } if elements.empty? return if params[:only_if_exists] == true