From 9a51e3e8f6b5786c31269d624c65110bf993e2ac Mon Sep 17 00:00:00 2001 From: Michael Grosser Date: Fri, 13 Dec 2024 08:31:25 -0800 Subject: [PATCH] fix perf copps --- .rubocop.yml | 7 ------- lib/css_parser.rb | 4 ++-- lib/css_parser/parser.rb | 22 ++++++++++++---------- lib/css_parser/rule_set.rb | 16 +++++++++------- test/test_css_parser_loading.rb | 2 +- 5 files changed, 24 insertions(+), 27 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 6624266..11c6145 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -42,13 +42,6 @@ Metrics/ModuleLength: Metrics/PerceivedComplexity: Enabled: false -# TODO: Progressively fix performance offences and remove this setting -Performance: - Enabled: false - -Performance/RegexpMatch: - Enabled: true - Style/AndOr: Enabled: false diff --git a/lib/css_parser.rb b/lib/css_parser.rb index 774245d..f62e180 100644 --- a/lib/css_parser.rb +++ b/lib/css_parser.rb @@ -57,7 +57,7 @@ def self.merge(*rule_sets) # in case called like CssParser.merge([rule_set, rule_set]) rule_sets.flatten! if rule_sets[0].is_a?(Array) - unless rule_sets.all? { |rs| rs.is_a?(CssParser::RuleSet) } + unless rule_sets.all?(CssParser::RuleSet) raise ArgumentError, 'all parameters must be CssParser::RuleSets.' end @@ -70,7 +70,7 @@ def self.merge(*rule_sets) rule_set.expand_shorthand! specificity = rule_set.specificity - specificity ||= rule_set.selectors.map { |s| calculate_specificity(s) }.compact.max || 0 + specificity ||= rule_set.selectors.filter_map { |s| calculate_specificity(s) }.max || 0 rule_set.each_declaration do |property, value, is_important| # Add the property to the list to be folded per http://www.w3.org/TR/CSS21/cascade.html#cascading-order diff --git a/lib/css_parser/parser.rb b/lib/css_parser/parser.rb index f027ce8..3ee7e4f 100644 --- a/lib/css_parser/parser.rb +++ b/lib/css_parser/parser.rb @@ -359,9 +359,9 @@ def parse_block_into_rule_sets!(block, options = {}) # :nodoc: in_at_media_rule = false in_media_block = false - current_selectors = String.new - current_media_query = String.new - current_declarations = String.new + current_selectors = +'' + current_media_query = +'' + current_declarations = +'' # once we are in a rule, we will use this to store where we started if we are capturing offsets rule_start = nil @@ -405,13 +405,14 @@ def parse_block_into_rule_sets!(block, options = {}) # :nodoc: media_types: current_media_queries } if options[:capture_offsets] - add_rule_options.merge!(filename: options[:filename], offset: rule_start..end_offset) + add_rule_options[:filename] = options[:filename] + add_rule_options[:offset] = rule_start..end_offset end add_rule!(**add_rule_options) end - current_selectors = String.new - current_declarations = String.new + current_selectors = +'' + current_declarations = +'' # restart our search for selectors and declarations rule_start = nil if options[:capture_offsets] @@ -426,14 +427,14 @@ def parse_block_into_rule_sets!(block, options = {}) # :nodoc: in_at_media_rule = false in_media_block = true current_media_queries << CssParser.sanitize_media_query(current_media_query) - current_media_query = String.new + current_media_query = +'' elsif token.include?(',') # new media query begins token.tr!(',', ' ') token.strip! current_media_query << token << ' ' current_media_queries << CssParser.sanitize_media_query(current_media_query) - current_media_query = String.new + current_media_query = +'' else token.strip! # special-case the ( and ) tokens to remove inner-whitespace @@ -478,7 +479,8 @@ def parse_block_into_rule_sets!(block, options = {}) # :nodoc: media_types: current_media_queries } if options[:capture_offsets] - add_rule_options.merge!(filename: options[:filename], offset: rule_start..end_offset) + add_rule_options[:filename] = options[:filename] + add_rule_options[:offset] = rule_start..end_offset end add_rule!(**add_rule_options) end @@ -718,7 +720,7 @@ def css_node_to_h(hash, key, val) nodes = {} lines.each do |line| parts = line.split(':', 2) - if /:/.match?(parts[1]) + if parts[1].include?(':') nodes[parts[0]] = css_node_to_h(hash, parts[0], parts[1]) else nodes[parts[0].to_s.strip] = parts[1].to_s.strip diff --git a/lib/css_parser/rule_set.rb b/lib/css_parser/rule_set.rb index d1bd4a8..5c06a92 100644 --- a/lib/css_parser/rule_set.rb +++ b/lib/css_parser/rule_set.rb @@ -11,8 +11,10 @@ class RuleSet BACKGROUND_PROPERTIES = ['background-color', 'background-image', 'background-repeat', 'background-position', 'background-size', 'background-attachment'].freeze LIST_STYLE_PROPERTIES = ['list-style-type', 'list-style-position', 'list-style-image'].freeze FONT_STYLE_PROPERTIES = ['font-style', 'font-variant', 'font-weight', 'font-size', 'line-height', 'font-family'].freeze + FONT_WEIGHT_PROPERTIES = ['font-style', 'font-weight', 'font-variant'].freeze BORDER_STYLE_PROPERTIES = ['border-width', 'border-style', 'border-color'].freeze BORDER_PROPERTIES = ['border', 'border-left', 'border-right', 'border-top', 'border-bottom'].freeze + DIMENSION_DIRECTIONS = [:top, :right, :bottom, :left].freeze NUMBER_OF_DIMENSIONS = 4 @@ -196,7 +198,7 @@ def replace_declaration!(property, replacements, preserve_importance: false) end def to_s(options = {}) - str = declarations.reduce(String.new) do |memo, (prop, value)| + str = declarations.reduce(+'') do |memo, (prop, value)| importance = options[:force_important] || value.important ? ' !important' : '' memo << "#{prop}: #{value.value}#{importance}; " end @@ -456,7 +458,7 @@ def expand_font_shorthand! # :nodoc: font_props['font-family'] = m end elsif /normal|inherit/i.match?(m) - ['font-style', 'font-weight', 'font-variant'].each do |font_prop| + FONT_WEIGHT_PROPERTIES.each do |font_prop| font_props[font_prop] ||= m end elsif /italic|oblique/i.match?(m) @@ -554,7 +556,7 @@ def create_background_shorthand! # :nodoc: # # TODO: this is extremely similar to create_background_shorthand! and should be combined def create_border_shorthand! # :nodoc: - values = BORDER_STYLE_PROPERTIES.map do |property| + values = BORDER_STYLE_PROPERTIES.filter_map do |property| next unless (declaration = declarations[property]) next if declaration.important # can't merge if any value contains a space (i.e. has multiple values) @@ -562,7 +564,7 @@ def create_border_shorthand! # :nodoc: next if /\s/.match?(declaration.value.gsub(/,\s/, ',').strip) declaration.value - end.compact + end return if values.size != BORDER_STYLE_PROPERTIES.size @@ -579,7 +581,7 @@ def create_dimensions_shorthand! # :nodoc: return if declarations.size < NUMBER_OF_DIMENSIONS DIMENSIONS.each do |property, dimensions| - values = [:top, :right, :bottom, :left].each_with_index.with_object({}) do |(side, index), result| + values = DIMENSION_DIRECTIONS.each_with_index.with_object({}) do |(side, index), result| next unless (declaration = declarations[dimensions[index]]) result[side] = declaration.value @@ -602,7 +604,7 @@ def create_dimensions_shorthand! # :nodoc: def create_font_shorthand! # :nodoc: return unless FONT_STYLE_PROPERTIES.all? { |prop| declarations.key?(prop) } - new_value = String.new + new_value = +'' ['font-style', 'font-variant', 'font-weight'].each do |property| unless declarations[property].value == 'normal' new_value << declarations[property].value << ' ' @@ -639,7 +641,7 @@ def compute_dimensions_shorthand(values) return [:top] if values.values.uniq.count == 1 # `/* top | right | bottom | left */` - return [:top, :right, :bottom, :left] if values[:left] != values[:right] + return DIMENSION_DIRECTIONS if values[:left] != values[:right] # Vertical are the same & horizontal are the same, `/* vertical | horizontal */` return [:top, :left] if values[:top] == values[:bottom] diff --git a/test/test_css_parser_loading.rb b/test/test_css_parser_loading.rb index 558ff47..3fac77e 100644 --- a/test/test_css_parser_loading.rb +++ b/test/test_css_parser_loading.rb @@ -141,7 +141,7 @@ def test_loading_malformed_content_strings file_name = File.expand_path('fixtures/import-malformed.css', __dir__) @cp.load_file!(file_name) @cp.each_selector do |_sel, dec, _spec| - assert_nil dec =~ /wellformed/ + assert_equal false, dec.include?('wellformed') end end