From 2c20cd6d62de70faf093009997c3fecf1d50f6d6 Mon Sep 17 00:00:00 2001 From: Francis Date: Wed, 5 Oct 2022 14:17:07 -0700 Subject: [PATCH 01/20] fix(STYLEGUIDE.md): remove extra opening code block In the `Keyword Arguments` section, there's an example that presents two code blocks. This change removes a lone opening code block without its matching end and adjusts the accompanying prose for readability. --- STYLEGUIDE.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/STYLEGUIDE.md b/STYLEGUIDE.md index b10d59c..c32650c 100644 --- a/STYLEGUIDE.md +++ b/STYLEGUIDE.md @@ -418,9 +418,8 @@ hsh = { [Keyword arguments](http://magazine.rubyist.net/?Ruby200SpecialEn-kwarg) are recommended but not required when a method's arguments may otherwise be opaque or non-obvious when called. Additionally, prefer them over the old "Hash as pseudo-named args" style from pre-2.0 ruby. [[link](#keyword-arguments)] -``` ruby - So instead of this: + ``` ruby def remove_member(user, skip_membership_check=false) # ... @@ -430,7 +429,8 @@ end remove_member(user, true) ``` -Do this, which is much clearer. +Do this, which is much clearer: + ``` ruby def remove_member(user, skip_membership_check: false) # ... @@ -893,4 +893,4 @@ result = hash.map { |_, v| v + 1 } Refactoring is even better. It's worth looking hard at any code that explicitly checks types. -[rubocop-guide]: https://github.com/rubocop-hq/ruby-style-guide \ No newline at end of file +[rubocop-guide]: https://github.com/rubocop-hq/ruby-style-guide From 1b549a305087169c3f730511c42093c4c67571bd Mon Sep 17 00:00:00 2001 From: Issy Long Date: Mon, 10 Oct 2022 18:54:28 +0000 Subject: [PATCH 02/20] config/default: Unset `DisabledByDefault: true` - The `DisabledByDefault` config option made it so that "all cops in the default configuration are disabled, and only cops in user configuration are enabled", which makes cops "opt-in rather than opt-out" (source: https://github.com/rubocop/rubocop/blob/d1270c468cdb9a211229e36aef8d568ae3bfe607/config/default.yml#L91-L102). - It's not immediately obvious that this gem has this config option, and it's used in a lot of GitHub Ruby projects. This means that people may write new, custom cops in downstream projects, with no configuration in `.rubocop.yml`, and then they never run because all cops are disabled unless explicitly enabled or otherwise configured with `Include`/`Exclude`. - This change is a follow-up to the great conversations had in https://github.com/github/rubocop-github/pull/117, and makes this gem take a more neutral stance to match whatever RuboCop's defaults are for enabling (or not) cops. Then it's up to the maintainers of this gem and the deciders of our styleguide to pick the settings for the rules to include, and/or it's up to the downstream consumers of this gem to decide whether they want all the rules or just some of the rules as they are defined in here. --- config/default.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/config/default.yml b/config/default.yml index 67af72a..f04a02b 100644 --- a/config/default.yml +++ b/config/default.yml @@ -2,9 +2,6 @@ require: - rubocop/cop/github - rubocop-performance -AllCops: - DisabledByDefault: true - Bundler/DuplicatedGem: Enabled: true From 7df97a71204a8bf09eb97f87818b5fc90de09e4f Mon Sep 17 00:00:00 2001 From: Issy Long Date: Mon, 10 Oct 2022 19:32:03 +0000 Subject: [PATCH 03/20] rubocop.yml: Disable some cops locally that aren't in the styleguide - This config applies only to this repo, it's not the global config. --- .rubocop.yml | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/.rubocop.yml b/.rubocop.yml index 802a6e6..593983b 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,6 +1,33 @@ inherit_from: ./config/default.yml +AllCops: + SuggestExtensions: false + +Gemspec/RequiredRubyVersion: + Enabled: false + +Lint/AssignmentInCondition: + Enabled: false + +Lint/EmptyConditionalBody: + Enabled: false + +Lint/UselessAssignment: + Enabled: false + Naming/FileName: Enabled: true Exclude: - "rubocop-github.gemspec" + +Style/Documentation: + Enabled: false + +Style/GuardClause: + Enabled: false + +Style/Next: + Enabled: false + +Style/SoleNestedConditional: + Enabled: false From 53a0a37784c3d95c05edbc3f57023072b00ef607 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Mon, 10 Oct 2022 19:32:38 +0000 Subject: [PATCH 04/20] Run `bundle exec rubocop -a` (or `-A`) on all the code in this repo - These rules seemed to make sense and were all autocorrectable: - Style/SymbolArray - Layout/EmptyLineAfterGuardClause - Style/MultipleComparison - Style/IfUnlessModifier (since we don't have line length limitations) - Style/EmptyCaseCondition - Style/TrailingUnderscoreVariable - Style/RedundantParentheses - Lint/UnusedBlockArgument - Style/NegatedIf - Style/StringConcatenation - Style/SafeNavigation - Layout/MultilineMethodCallIndentation - Layout/EmptyLineAfterMagicComment - Layout/SpaceInsideHashLiteralBraces - Layout/EmptyLines - Layout/EmptyLineBetweenDefs --- Rakefile | 2 +- .../cop/github/insecure_hash_algorithm.rb | 31 +++++++------------ .../cop/github/rails_application_record.rb | 6 ++-- .../rails_controller_render_action_symbol.rb | 2 +- .../github/rails_controller_render_literal.rb | 14 +++------ .../rails_controller_render_paths_exist.rb | 14 +++------ .../rails_controller_render_shorthand.rb | 4 +-- lib/rubocop/cop/github/rails_render_inline.rb | 4 +-- .../github/rails_render_object_collection.rb | 4 +-- .../cop/github/rails_view_render_literal.rb | 10 ++---- .../github/rails_view_render_paths_exist.rb | 10 ++---- .../cop/github/render_literal_helpers.rb | 2 +- test/test_insecure_hash_algorithm.rb | 2 +- test/test_rails_controller_render_literal.rb | 1 - 14 files changed, 37 insertions(+), 69 deletions(-) diff --git a/Rakefile b/Rakefile index 25e3326..66dceb3 100644 --- a/Rakefile +++ b/Rakefile @@ -4,7 +4,7 @@ require "bundler/gem_tasks" require "rake/testtask" require "rubocop/rake_task" -task default: [:test, :rubocop] +task default: %i[test rubocop] Rake::TestTask.new RuboCop::RakeTask.new diff --git a/lib/rubocop/cop/github/insecure_hash_algorithm.rb b/lib/rubocop/cop/github/insecure_hash_algorithm.rb index de1b321..032bc12 100644 --- a/lib/rubocop/cop/github/insecure_hash_algorithm.rb +++ b/lib/rubocop/cop/github/insecure_hash_algorithm.rb @@ -64,6 +64,7 @@ class InsecureHashAlgorithm < Base def insecure_algorithm?(val) return false if val == :Digest # Don't match "Digest::Digest". + case alg_name(val) when *allowed_hash_functions false @@ -80,7 +81,7 @@ def not_just_encoding?(val) end def just_encoding?(val) - val == :hexencode || val == :bubblebabble + %i[hexencode bubblebabble].include?(val) end # Built-in hash functions are listed in these docs: @@ -99,6 +100,7 @@ def allowed_hash_functions def alg_name(val) return :nil if val.nil? return val.to_s.downcase unless val.is_a?(RuboCop::AST::Node) + case val.type when :sym, :str val.children.first.to_s.downcase @@ -108,28 +110,19 @@ def alg_name(val) end def on_const(const_node) - if insecure_const?(const_node) && !digest_uuid?(const_node) - add_offense(const_node, message: MSG) - end + add_offense(const_node, message: MSG) if insecure_const?(const_node) && !digest_uuid?(const_node) end def on_send(send_node) - case - when uuid_v3?(send_node) - unless allowed_hash_functions.include?("md5") - add_offense(send_node, message: UUID_V3_MSG) - end - when uuid_v5?(send_node) - unless allowed_hash_functions.include?("sha1") - add_offense(send_node, message: UUID_V5_MSG) - end - when openssl_hmac_new?(send_node) - if openssl_hmac_new_insecure?(send_node) - add_offense(send_node, message: MSG) - end - when insecure_digest?(send_node) + if uuid_v3?(send_node) + add_offense(send_node, message: UUID_V3_MSG) unless allowed_hash_functions.include?("md5") + elsif uuid_v5?(send_node) + add_offense(send_node, message: UUID_V5_MSG) unless allowed_hash_functions.include?("sha1") + elsif openssl_hmac_new?(send_node) + add_offense(send_node, message: MSG) if openssl_hmac_new_insecure?(send_node) + elsif insecure_digest?(send_node) add_offense(send_node, message: MSG) - when insecure_hash_lookup?(send_node) + elsif insecure_hash_lookup?(send_node) add_offense(send_node, message: MSG) end end diff --git a/lib/rubocop/cop/github/rails_application_record.rb b/lib/rubocop/cop/github/rails_application_record.rb index 1ec30cf..cfb1974 100644 --- a/lib/rubocop/cop/github/rails_application_record.rb +++ b/lib/rubocop/cop/github/rails_application_record.rb @@ -17,11 +17,9 @@ class RailsApplicationRecord < Base PATTERN def on_class(node) - klass, superclass, _ = *node + klass, superclass, = *node - if active_record_base_const?(superclass) && !(application_record_const?(klass)) - add_offense(superclass) - end + add_offense(superclass) if active_record_base_const?(superclass) && !application_record_const?(klass) end end end diff --git a/lib/rubocop/cop/github/rails_controller_render_action_symbol.rb b/lib/rubocop/cop/github/rails_controller_render_action_symbol.rb index e3d6eca..8661833 100644 --- a/lib/rubocop/cop/github/rails_controller_render_action_symbol.rb +++ b/lib/rubocop/cop/github/rails_controller_render_action_symbol.rb @@ -24,7 +24,7 @@ class RailsControllerRenderActionSymbol < Base def on_send(node) if sym_node = render_sym?(node) - add_offense(sym_node) do |corrector| + add_offense(sym_node) do |_corrector| register_offense(sym_node, node) end elsif option_pairs = render_with_options?(node) diff --git a/lib/rubocop/cop/github/rails_controller_render_literal.rb b/lib/rubocop/cop/github/rails_controller_render_literal.rb index 0f2efd3..edda84d 100644 --- a/lib/rubocop/cop/github/rails_controller_render_literal.rb +++ b/lib/rubocop/cop/github/rails_controller_render_literal.rb @@ -60,12 +60,10 @@ def on_send(node) elsif option_pairs = render_with_options?(node) option_pairs = option_pairs.reject { |pair| options_key?(pair) } - if option_pairs.any? { |pair| ignore_key?(pair) } - return - end + return if option_pairs.any? { |pair| ignore_key?(pair) } if template_node = option_pairs.map { |pair| template_key?(pair) }.compact.first - if !literal?(template_node) + unless literal?(template_node) add_offense(node) return end @@ -75,7 +73,7 @@ def on_send(node) end if layout_node = option_pairs.map { |pair| layout_key?(pair) }.compact.first - if !literal?(layout_node) + unless literal?(layout_node) add_offense(node) return end @@ -91,16 +89,14 @@ def on_send(node) add_offense(node) return end - option_pairs = option_hash && option_hash.pairs + option_pairs = option_hash&.pairs else option_pairs = node.arguments[0].pairs end if option_pairs locals = option_pairs.map { |pair| locals_key?(pair) }.compact.first - if locals && (!locals.hash_type? || !hash_with_literal_keys?(locals)) - add_offense(node) - end + add_offense(node) if locals && (!locals.hash_type? || !hash_with_literal_keys?(locals)) end end end diff --git a/lib/rubocop/cop/github/rails_controller_render_paths_exist.rb b/lib/rubocop/cop/github/rails_controller_render_paths_exist.rb index 126738a..3f84ce2 100644 --- a/lib/rubocop/cop/github/rails_controller_render_paths_exist.rb +++ b/lib/rubocop/cop/github/rails_controller_render_paths_exist.rb @@ -27,22 +27,16 @@ def on_send(node) if args = render_str?(node) node, path = args - unless resolve_template(path.to_s) - add_offense(node, message: "Template could not be found") - end + add_offense(node, message: "Template could not be found") unless resolve_template(path.to_s) elsif pairs = render_options?(node) if pair = pairs.detect { |p| render_key?(p) } key, node, path = render_key?(pair) case key when :action, :template - unless resolve_template(path.to_s) - add_offense(node, message: "Template could not be found") - end + add_offense(node, message: "Template could not be found") unless resolve_template(path.to_s) when :partial - unless resolve_partial(path.to_s) - add_offense(node, message: "Partial template could not be found") - end + add_offense(node, message: "Partial template could not be found") unless resolve_partial(path.to_s) end end end @@ -50,7 +44,7 @@ def on_send(node) def resolve_template(path) cop_config["ViewPath"].each do |view_path| - if m = Dir[File.join(config.path_relative_to_config(view_path), path) + "*"].first + if m = Dir["#{File.join(config.path_relative_to_config(view_path), path)}*"].first return m end end diff --git a/lib/rubocop/cop/github/rails_controller_render_shorthand.rb b/lib/rubocop/cop/github/rails_controller_render_shorthand.rb index 58069e1..ae62f71 100644 --- a/lib/rubocop/cop/github/rails_controller_render_shorthand.rb +++ b/lib/rubocop/cop/github/rails_controller_render_shorthand.rb @@ -28,8 +28,8 @@ def on_send(node) if value_node = action_key?(pair) comma = option_pairs.length > 1 ? ", " : "" corrected_source = node.source - .sub(/#{pair.source}(,\s*)?/, "") - .sub("render ", "render \"#{str(value_node)}\"#{comma}") + .sub(/#{pair.source}(,\s*)?/, "") + .sub("render ", "render \"#{str(value_node)}\"#{comma}") add_offense(node, message: "Use `#{corrected_source}` instead") do |corrector| corrector.replace(node.source_range, corrected_source) diff --git a/lib/rubocop/cop/github/rails_render_inline.rb b/lib/rubocop/cop/github/rails_render_inline.rb index 8f76fb2..65c8fb5 100644 --- a/lib/rubocop/cop/github/rails_render_inline.rb +++ b/lib/rubocop/cop/github/rails_render_inline.rb @@ -18,9 +18,7 @@ class RailsRenderInline < Base def on_send(node) if option_pairs = render_with_options?(node) - if option_pairs.detect { |pair| inline_key?(pair) } - add_offense(node) - end + add_offense(node) if option_pairs.detect { |pair| inline_key?(pair) } end end end diff --git a/lib/rubocop/cop/github/rails_render_object_collection.rb b/lib/rubocop/cop/github/rails_render_object_collection.rb index 38a27fa..b5a13b5 100644 --- a/lib/rubocop/cop/github/rails_render_object_collection.rb +++ b/lib/rubocop/cop/github/rails_render_object_collection.rb @@ -31,9 +31,7 @@ def on_send(node) case object_sym when :object - if partial_name.children[0].is_a?(String) - suggestion = ", instead `render partial: #{partial_name.source}, locals: { #{File.basename(partial_name.children[0], '.html.erb')}: #{object_node.source} }`" - end + suggestion = ", instead `render partial: #{partial_name.source}, locals: { #{File.basename(partial_name.children[0], '.html.erb')}: #{object_node.source} }`" if partial_name.children[0].is_a?(String) add_offense(node, message: "Avoid `render object:`#{suggestion}") when :collection, :spacer_template add_offense(node, message: "Avoid `render collection:`") diff --git a/lib/rubocop/cop/github/rails_view_render_literal.rb b/lib/rubocop/cop/github/rails_view_render_literal.rb index c856386..d3070d3 100644 --- a/lib/rubocop/cop/github/rails_view_render_literal.rb +++ b/lib/rubocop/cop/github/rails_view_render_literal.rb @@ -34,12 +34,10 @@ def on_send(node) if render_literal?(node) elsif option_pairs = render_with_options?(node) - if option_pairs.any? { |pair| ignore_key?(pair) } - return - end + return if option_pairs.any? { |pair| ignore_key?(pair) } if partial_node = option_pairs.map { |pair| partial_key?(pair) }.compact.first - if !literal?(partial_node) + unless literal?(partial_node) add_offense(node) return end @@ -60,9 +58,7 @@ def on_send(node) if locals if locals.hash_type? - if !hash_with_literal_keys?(locals) - add_offense(node) - end + add_offense(node) unless hash_with_literal_keys?(locals) else add_offense(node) end diff --git a/lib/rubocop/cop/github/rails_view_render_paths_exist.rb b/lib/rubocop/cop/github/rails_view_render_paths_exist.rb index 49a40f2..6423416 100644 --- a/lib/rubocop/cop/github/rails_view_render_paths_exist.rb +++ b/lib/rubocop/cop/github/rails_view_render_paths_exist.rb @@ -27,16 +27,12 @@ def on_send(node) if args = render_str?(node) node, path = args - unless resolve_partial(path.to_s) - add_offense(node, message: "Partial template could not be found") - end + add_offense(node, message: "Partial template could not be found") unless resolve_partial(path.to_s) elsif pairs = render_options?(node) if pair = pairs.detect { |p| partial_key?(p) } node, path = partial_key?(pair) - unless resolve_partial(path.to_s) - add_offense(node, message: "Partial template could not be found") - end + add_offense(node, message: "Partial template could not be found") unless resolve_partial(path.to_s) end end end @@ -47,7 +43,7 @@ def resolve_partial(path) path = parts.join(File::SEPARATOR) cop_config["ViewPath"].each do |view_path| - if m = Dir[File.join(config.path_relative_to_config(view_path), path) + "*"].first + if m = Dir["#{File.join(config.path_relative_to_config(view_path), path)}*"].first return m end end diff --git a/lib/rubocop/cop/github/render_literal_helpers.rb b/lib/rubocop/cop/github/render_literal_helpers.rb index 67417a5..6994fb7 100644 --- a/lib/rubocop/cop/github/render_literal_helpers.rb +++ b/lib/rubocop/cop/github/render_literal_helpers.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true -# + require "rubocop" module RuboCop diff --git a/test/test_insecure_hash_algorithm.rb b/test/test_insecure_hash_algorithm.rb index 597450b..3e48aa9 100644 --- a/test/test_insecure_hash_algorithm.rb +++ b/test/test_insecure_hash_algorithm.rb @@ -10,7 +10,7 @@ def cop_class end def make_cop(allowed:) - config = RuboCop::Config.new({"GitHub/InsecureHashAlgorithm" => {"Allowed" => allowed}}) + config = RuboCop::Config.new({ "GitHub/InsecureHashAlgorithm" => { "Allowed" => allowed } }) cop_class.new(config) end diff --git a/test/test_rails_controller_render_literal.rb b/test/test_rails_controller_render_literal.rb index 9da2f16..bd52c7f 100644 --- a/test/test_rails_controller_render_literal.rb +++ b/test/test_rails_controller_render_literal.rb @@ -442,7 +442,6 @@ def index assert_equal 1, offenses.count end - def test_render_literal_dynamic_local_key_offense offenses = investigate cop, <<-RUBY, "app/controllers/products_controller.rb" class ProductsController < ActionController::Base From e36d683f6b85e7394775a02d9f6321f2cf34b4b5 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Mon, 10 Oct 2022 19:41:45 +0000 Subject: [PATCH 05/20] Fix `Naming/MemoizedInstanceVariableName` offenses --- lib/rubocop/cop/github/insecure_hash_algorithm.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/rubocop/cop/github/insecure_hash_algorithm.rb b/lib/rubocop/cop/github/insecure_hash_algorithm.rb index 032bc12..e9fb330 100644 --- a/lib/rubocop/cop/github/insecure_hash_algorithm.rb +++ b/lib/rubocop/cop/github/insecure_hash_algorithm.rb @@ -94,7 +94,7 @@ def just_encoding?(val) ].freeze def allowed_hash_functions - @allowed_algorithms ||= cop_config.fetch("Allowed", DEFAULT_ALLOWED).map(&:downcase) + @allowed_hash_functions ||= cop_config.fetch("Allowed", DEFAULT_ALLOWED).map(&:downcase) end def alg_name(val) From a899fa276edf38e3ce20c5c59a61002b697d82f5 Mon Sep 17 00:00:00 2001 From: Issy Long Date: Tue, 11 Oct 2022 16:04:26 +0000 Subject: [PATCH 06/20] README, guides: Clean up leftover `GitHub/Accessibility` cop references - My changes in https://github.com/github/rubocop-github/pull/118 didn't quite go far enough. --- README.md | 2 +- guides/image-has-alt.md | 29 ----------------------------- guides/link-has-href.md | 29 ----------------------------- guides/no-positive-tabindex.md | 24 ------------------------ guides/no-redundant-image-alt.md | 24 ------------------------ 5 files changed, 1 insertion(+), 107 deletions(-) delete mode 100644 guides/image-has-alt.md delete mode 100644 guides/link-has-href.md delete mode 100644 guides/no-positive-tabindex.md delete mode 100644 guides/no-redundant-image-alt.md diff --git a/README.md b/README.md index b5fb2ad..da68674 100644 --- a/README.md +++ b/README.md @@ -37,4 +37,4 @@ bundle exec rake test ## The Cops -All cops are located under [`lib/rubocop/cop/github`](lib/rubocop/cop/github) and [`lib/rubocop/cop/github/accessibility`](lib/rubocop/cop/github/accessibility), and contain examples/documentation. +All cops are located under [`lib/rubocop/cop/github`](lib/rubocop/cop/github). diff --git a/guides/image-has-alt.md b/guides/image-has-alt.md deleted file mode 100644 index a761dba..0000000 --- a/guides/image-has-alt.md +++ /dev/null @@ -1,29 +0,0 @@ -# GitHub/Accessibility/ImageHasAlt - -## Rule Details - -Images should have an alt prop with meaningful text or an empty string for decorative images. - -## Resources - -- [W3C WAI Images Tutorial](https://www.w3.org/WAI/tutorials/images/) -- [Primer: Alternative text for images](https://primer.style/design/accessibility/alternative-text-for-images) - -## Examples -### **Incorrect** code for this rule 👎 - -```erb -<%= image_tag "spinners/octocat-spinner-16px.gif", size: "12x12" %> -``` - -### **Correct** code for this rule 👍 - -```erb - -<%= image_tag "spinners/octocat-spinner-16px.gif", size: "12x12", alt: "GitHub Logo spinner" %> -``` - -```erb - -<%= image_tag "spinners/octocat-spinner-16px.gif", size: "12x12", alt: "" %> -``` \ No newline at end of file diff --git a/guides/link-has-href.md b/guides/link-has-href.md deleted file mode 100644 index 8a574c9..0000000 --- a/guides/link-has-href.md +++ /dev/null @@ -1,29 +0,0 @@ -# GitHub/Accessibility/LinkHasHref - -## Rule Details - -Links should go somewhere, you probably want to use a `