Merge pull request premailer#154 from stoivo/st-refactor-creation-of-…

…rule-set

Changes to creation of rule set

.github/workflows/ci.yml

@@ -35,7 +35,7 @@ jobs:
 
       - uses: ruby/setup-ruby@v1
         with:
-          ruby-version: '2.7'
+          ruby-version: '3.0'
           bundler-cache: true
 
       - run: bundle exec rake rubocop

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ### Unreleased
 
+* Deprecate `add_rule!` (positional arguments)and `add_rule_with_offsets!` for `add_rule!` (keyword argument)
+* RuleSet initialize now takes keyword argument, positional arguments are still supported but deprecated
+* Removed OffsetAwareRuleSet, it's a RuleSet with optional attributes filename and offset
+
 ### Version v1.18.0
 
  * Drop Ruby 2.7 compatibility for parity with Premailer [#149](https://github.com/premailer/css_parser/pull/149)

Rakefile

@@ -13,7 +13,11 @@ Rake::TestTask.new do |test|
   test.verbose = true
 end
 
-RuboCop::RakeTask.new
+RuboCop::RakeTask.new do |t|
+  # allow you to run "$ rake rubocop -a" to autofix
+  t.options << '-a' if ARGV.include?('-a')
+  t.options << '-A' if ARGV.include?('-A')
+end
 
 desc 'Run a performance evaluation.'
 task :benchmark do

lib/css_parser/parser.rb

@@ -164,14 +164,44 @@ def add_block!(block, options = {})
       parse_block_into_rule_sets!(block, options)
     end
 
-    # Add a CSS rule by setting the +selectors+, +declarations+ and +media_types+.
+    # Add a CSS rule by setting the +selectors+, +declarations+
+    # and +media_types+. Optional pass +filename+ , +offset+ for source
+    # reference too.
     #
-    # +media_types+ can be a symbol or an array of symbols.
-    def add_rule!(selectors, declarations, media_types = :all)
-      rule_set = RuleSet.new(selectors, declarations)
-      add_rule_set!(rule_set, media_types)
-    rescue ArgumentError => e
-      raise e if @options[:rule_set_exceptions]
+    # +media_types+ can be a symbol or an array of symbols. default to :all
+    # optional fields for source location for source location
+    # +filename+ can be a string or uri pointing to the file or url location.
+    # +offset+ should be Range object representing the start and end byte locations where the rule was found in the file.
+    def add_rule!(*args, selectors: nil, block: nil, filename: nil, offset: nil, media_types: :all) # rubocop:disable Metrics/ParameterLists
+      if args.any?
+        media_types = nil
+        if selectors || block || filename || offset || media_types
+          raise ArgumentError, "don't mix positional and keyword arguments arguments"
+        end
+
+        warn '[DEPRECATION] `add_rule!` with positional arguments is deprecated. ' \
+             'Please use keyword arguments instead.', uplevel: 1
+
+        case args.length
+        when 2
+          selectors, block = args
+        when 3
+          selectors, block, media_types = args
+        else
+          raise ArgumentError
+        end
+      end
+
+      begin
+        rule_set = RuleSet.new(
+          selectors: selectors, block: block,
+          offset: offset, filename: filename
+        )
+
+        add_rule_set!(rule_set, media_types)
+      rescue ArgumentError => e
+        raise e if @options[:rule_set_exceptions]
+      end
     end
 
     # Add a CSS rule by setting the +selectors+, +declarations+, +filename+, +offset+ and +media_types+.
@@ -180,8 +210,11 @@ def add_rule!(selectors, declarations, media_types = :all)
     # +offset+ should be Range object representing the start and end byte locations where the rule was found in the file.
     # +media_types+ can be a symbol or an array of symbols.
     def add_rule_with_offsets!(selectors, declarations, filename, offset, media_types = :all)
-      rule_set = OffsetAwareRuleSet.new(filename, offset, selectors, declarations)
-      add_rule_set!(rule_set, media_types)
+      warn '[DEPRECATION] `add_rule_with_offsets!` is deprecated. Please use `add_rule!` instead.', uplevel: 1
+      add_rule!(
+        selectors: selectors, block: declarations, media_types: media_types,
+        filename: filename, offset: offset
+      )
     end
 
     # Add a CssParser RuleSet object.
@@ -360,11 +393,14 @@ def parse_block_into_rule_sets!(block, options = {}) # :nodoc:
             current_declarations.strip!
 
             unless current_declarations.empty?
+              add_rule_options = {
+                selectors: current_selectors, block: current_declarations,
+                media_types: current_media_queries
+              }
               if options[:capture_offsets]
-                add_rule_with_offsets!(current_selectors, current_declarations, options[:filename], (rule_start..offset.last), current_media_queries)
-              else
-                add_rule!(current_selectors, current_declarations, current_media_queries)
+                add_rule_options.merge!(filename: options[:filename], offset: rule_start..offset.last)
               end
+              add_rule!(**add_rule_options)
             end
 
             current_selectors = String.new
@@ -430,11 +466,14 @@ def parse_block_into_rule_sets!(block, options = {}) # :nodoc:
       # check for unclosed braces
       return unless in_declarations > 0
 
-      unless options[:capture_offsets]
-        return add_rule!(current_selectors, current_declarations, current_media_queries)
+      add_rule_options = {
+        selectors: current_selectors, block: current_declarations,
+        media_types: current_media_queries
+      }
+      if options[:capture_offsets]
+        add_rule_options.merge!(filename: options[:filename], offset: rule_start..offset.last)
       end
-
-      add_rule_with_offsets!(current_selectors, current_declarations, options[:filename], (rule_start..offset.last), current_media_queries)
+      add_rule!(**add_rule_options)
     end
 
     # Load a remote CSS file.

lib/css_parser/rule_set.rb

@@ -223,6 +223,12 @@ def normalize_property(property)
 
     extend Forwardable
 
+    # optional field for storing source reference
+    # File offset range
+    attr_reader :offset
+    # the local or remote location
+    attr_accessor :filename
+
     # Array of selector strings.
     attr_reader :selectors
 
@@ -237,9 +243,38 @@ def normalize_property(property)
     alias []= add_declaration!
     alias remove_declaration! delete
 
-    def initialize(selectors, block, specificity = nil)
+    def initialize(*args, selectors: nil, block: nil, offset: nil, filename: nil, specificity: nil) # rubocop:disable Metrics/ParameterLists
+      if args.any?
+        if selectors || block || offset || filename || specificity
+          raise ArgumentError, "don't mix positional and keyword arguments"
+        end
+
+        warn '[DEPRECATION] positional arguments are deprecated use keyword instead.', uplevel: 1
+
+        case args.length
+        when 2
+          selectors, block = args
+        when 3
+          selectors, block, specificity = args
+        when 4
+          filename, offset, selectors, block = args
+        when 5
+          filename, offset, selectors, block, specificity = args
+        else
+          raise ArgumentError
+        end
+      end
+
       @selectors = []
       @specificity = specificity
+
+      unless offset.nil? == filename.nil?
+        raise ArgumentError, 'require both offset and filename or no offset and no filename'
+      end
+
+      @offset = offset
+      @filename = filename
+
       parse_selectors!(selectors) if selectors
       parse_declarations!(block)
     end
@@ -650,18 +685,4 @@ def split_value_preserving_function_whitespace(value)
       end
     end
   end
-
-  class OffsetAwareRuleSet < RuleSet
-    # File offset range
-    attr_reader :offset
-
-    # the local or remote location
-    attr_accessor :filename
-
-    def initialize(filename, offset, selectors, block, specificity = nil)
-      super(selectors, block, specificity)
-      @offset = offset
-      @filename = filename
-    end
-  end
 end

test/test_css_parser_basic.rb

@@ -35,20 +35,20 @@ def test_adding_block_without_closing_brace
   end
 
   def test_adding_a_rule
-    @cp.add_rule!('div', 'color: blue;')
+    @cp.add_rule!(selectors: 'div', block: 'color: blue;')
     assert_equal 'color: blue;', @cp.find_by_selector('div').join(' ')
   end
 
   def test_adding_a_rule_set
-    rs = CssParser::RuleSet.new('div', 'color: blue;')
+    rs = CssParser::RuleSet.new(selectors: 'div', block: 'color: blue;')
     @cp.add_rule_set!(rs)
     assert_equal 'color: blue;', @cp.find_by_selector('div').join(' ')
   end
 
   def test_removing_a_rule_set
-    rs = CssParser::RuleSet.new('div', 'color: blue;')
+    rs = CssParser::RuleSet.new(selectors: 'div', block: 'color: blue;')
     @cp.add_rule_set!(rs)
-    rs2 = CssParser::RuleSet.new('div', 'color: blue;')
+    rs2 = CssParser::RuleSet.new(selectors: 'div', block: 'color: blue;')
     @cp.remove_rule_set!(rs2)
     assert_equal '', @cp.find_by_selector('div').join(' ')
   end
@@ -72,7 +72,7 @@ def test_toggling_uri_conversion
   end
 
   def test_converting_to_hash
-    rs = CssParser::RuleSet.new('div', 'color: blue;')
+    rs = CssParser::RuleSet.new(selectors: 'div', block: 'color: blue;')
     @cp.add_rule_set!(rs)
     hash = @cp.to_h
     assert_equal 'blue', hash['all']['div']['color']

test/test_css_parser_loading.rb

@@ -204,16 +204,4 @@ def test_toggling_not_found_exceptions
 
     cp_without_exceptions.load_uri!("#{@uri_base}/no-exist.xyz")
   end
-
-  def test_rule_set_argument_exceptions
-    cp_with_exceptions = Parser.new(rule_set_exceptions: true)
-
-    assert_raises ArgumentError, 'background-color value is empty' do
-      cp_with_exceptions.add_rule!('body', 'background-color: !important')
-    end
-
-    cp_without_exceptions = Parser.new(rule_set_exceptions: false)
-
-    cp_without_exceptions.add_rule!('body', 'background-color: !important')
-  end
 end

test/test_css_parser_media_types.rb

@@ -135,24 +135,24 @@ def test_adding_block_and_limiting_media_types
   end
 
   def test_adding_rule_set_with_media_type
-    @cp.add_rule!('body', 'color: black;', [:handheld, :tty])
-    @cp.add_rule!('body', 'color: blue;', :screen)
+    @cp.add_rule!(selectors: 'body', block: 'color: black;', media_types: [:handheld, :tty])
+    @cp.add_rule!(selectors: 'body', block: 'color: blue;', media_types: :screen)
     assert_equal 'color: black;', @cp.find_by_selector('body', :handheld).join(' ')
   end
 
   def test_adding_rule_set_with_media_query
-    @cp.add_rule!('body', 'color: black;', 'aural and (device-aspect-ratio: 16/9)')
+    @cp.add_rule!(selectors: 'body', block: 'color: black;', media_types: 'aural and (device-aspect-ratio: 16/9)')
     assert_equal 'color: black;', @cp.find_by_selector('body', 'aural and (device-aspect-ratio: 16/9)').join(' ')
     assert_equal 'color: black;', @cp.find_by_selector('body', :all).join(' ')
   end
 
   def test_selecting_with_all_media_types
-    @cp.add_rule!('body', 'color: black;', [:handheld, :tty])
+    @cp.add_rule!(selectors: 'body', block: 'color: black;', media_types: [:handheld, :tty])
     assert_equal 'color: black;', @cp.find_by_selector('body', :all).join(' ')
   end
 
   def test_to_s_includes_media_queries
-    @cp.add_rule!('body', 'color: black;', 'aural and (device-aspect-ratio: 16/9)')
+    @cp.add_rule!(selectors: 'body', block: 'color: black;', media_types: 'aural and (device-aspect-ratio: 16/9)')
     assert_equal "@media aural and (device-aspect-ratio: 16/9) {\n  body {\n    color: black;\n  }\n}\n", @cp.to_s
   end
 end

test/test_css_parser_misc.rb

@@ -195,20 +195,20 @@ def test_ruleset_with_braces
     # rulesets = []
     #
     # parser['div'].each do |declaration|
-    #   rulesets << RuleSet.new('div', declaration)
+    #   rulesets << RuleSet.new(selectors: 'div', block: declaration)
     # end
     #
     # merged = CssParser.merge(rulesets)
     #
     # result: # merged.to_s => "{ background-color: black !important; }"
 
-    new_rule = RuleSet.new('div', "{ background-color: black !important; }")
+    new_rule = RuleSet.new(selectors: 'div', block: "{ background-color: black !important; }")
 
     assert_equal 'div { background-color: black !important; }', new_rule.to_s
   end
 
   def test_content_with_data
-    rule = RuleSet.new('div', '{content: url(data:image/png;base64,LOTSOFSTUFF)}')
+    rule = RuleSet.new(selectors: 'div', block: '{content: url(data:image/png;base64,LOTSOFSTUFF)}')
     assert_includes rule.to_s, "image/png;base64,LOTSOFSTUFF"
   end
 
@@ -226,4 +226,48 @@ def test_enumerator_nonempty
       assert_equal 'color: black;', desc
     end
   end
+
+  def test_catching_argument_exceptions_for_add_rule
+    cp_with_exceptions = Parser.new(rule_set_exceptions: true)
+    assert_raises ArgumentError, 'background-color value is empty' do
+      cp_with_exceptions.add_rule!(selectors: 'body', block: 'background-color: !important')
+    end
+
+    cp_without_exceptions = Parser.new(rule_set_exceptions: false)
+    cp_without_exceptions.add_rule!(selectors: 'body', block: 'background-color: !important')
+  end
+
+  def test_catching_argument_exceptions_for_add_rule_positional
+    cp_with_exceptions = Parser.new(rule_set_exceptions: true)
+
+    assert_raises ArgumentError, 'background-color value is empty' do
+      _, err = capture_io do
+        cp_with_exceptions.add_rule!('body', 'background-color: !important')
+      end
+      assert_includes err, "DEPRECATION"
+    end
+
+    cp_without_exceptions = Parser.new(rule_set_exceptions: false)
+    _, err = capture_io do
+      cp_without_exceptions.add_rule!('body', 'background-color: !important')
+    end
+    assert_includes err, "DEPRECATION"
+  end
+
+  def test_catching_argument_exceptions_for_add_rule_with_offsets
+    cp_with_exceptions = Parser.new(capture_offsets: true, rule_set_exceptions: true)
+
+    assert_raises ArgumentError, 'background-color value is empty' do
+      _, err = capture_io do
+        cp_with_exceptions.add_rule_with_offsets!('body', 'background-color: !important', 'inline', 1)
+      end
+      assert_includes err, "DEPRECATION"
+    end
+
+    cp_without_exceptions = Parser.new(capture_offsets: true, rule_set_exceptions: false)
+    _, err = capture_io do
+      cp_without_exceptions.add_rule_with_offsets!('body', 'background-color: !important', 'inline', 1)
+    end
+    assert_includes err, "DEPRECATION"
+  end
 end