Merge pull request #545 from seuros/taglistparser

Deprecated Taglist.parse and fix #544

acts-as-taggable-on.gemspec

@@ -32,7 +32,6 @@ Gem::Specification.new do |gem|
   gem.add_development_dependency 'rspec-rails'  , '~> 3.0.0.beta'
   gem.add_development_dependency 'rspec-its'
   gem.add_development_dependency 'rspec'
-  gem.add_development_dependency 'ammeter'
   gem.add_development_dependency 'barrier'
   gem.add_development_dependency 'database_cleaner'
 end

lib/acts-as-taggable-on.rb

@@ -11,6 +11,7 @@ module ActsAsTaggableOn
   autoload :Engine
   autoload :Tag
   autoload :TagList
+  autoload :TagListParser
   autoload :Taggable
   autoload :Tagger
   autoload :Tagging

lib/acts_as_taggable_on/tag_list.rb

@@ -1,3 +1,4 @@
+
 require 'active_support/core_ext/module/delegation'
 
 module ActsAsTaggableOn
@@ -8,82 +9,6 @@ def initialize(*args)
       add(*args)
     end
 
-    class << self
-      ##
-      # Returns a new TagList using the given tag string.
-      #
-      # Example:
-      #   tag_list = ActsAsTaggableOn::TagList.from("One , Two,  Three")
-      #   tag_list # ["One", "Two", "Three"]
-      def from(string)
-        string = string.join(ActsAsTaggableOn.glue) if string.respond_to?(:join)
-
-        new.tap do |tag_list|
-          string = string.to_s.dup
-
-
-          string.gsub!(double_quote_pattern) {
-            # Append the matched tag to the tag list
-            tag_list << Regexp.last_match[2]
-            # Return the matched delimiter ($3) to replace the matched items
-            ''
-          }
-
-          string.gsub!(single_quote_pattern) {
-            # Append the matched tag ($2) to the tag list
-            tag_list << Regexp.last_match[2]
-            # Return an empty string to replace the matched items
-            ''
-          }
-
-          # split the string by the delimiter
-          # and add to the tag_list
-          tag_list.add(string.split(Regexp.new delimiter))
-        end
-      end
-
-      def delimiter
-        # Parse the quoted tags
-        d = ActsAsTaggableOn.delimiter
-        # Separate multiple delimiters by bitwise operator
-        d = d.join('|') if d.kind_of?(Array)
-
-        d
-      end
-
-      def single_quote_pattern
-        %r{
-          (             # Tag start delimiter ($1)
-            \A       |  # Either string start or
-            #{delimiter}        # a delimiter
-          )
-          \s*'          # quote (') optionally preceded by whitespace
-          (.*?)         # Tag ($2)
-          '\s*          # quote (') optionally followed by whitespace
-          (?=           # Tag end delimiter (not consumed; is zero-length lookahead)
-            #{delimiter}\s*  |  # Either a delimiter optionally followed by whitespace or
-            \z          # string end
-          )
-      }x
-      end
-
-      def double_quote_pattern
-        %r{
-          (             # Tag start delimiter ($1)
-            \A       |  # Either string start or
-            #{delimiter}        # a delimiter
-          )
-          \s*"          # quote (") optionally preceded by whitespace
-          (.*?)         # Tag ($2)
-          "\s*          # quote (") optionally followed by whitespace
-          (?=           # Tag end delimiter (not consumed; is zero-length lookahead)
-            #{delimiter}\s*  |  # Either a delimiter optionally followed by whitespace or
-            \z          # string end
-          )
-      }x
-      end
-
-    end
     ##
     # Add tags to the tag_list. Duplicate or blank tags will be ignored.
     # Use the <tt>:parse</tt> option to add an unparsed tag string.
@@ -165,12 +90,23 @@ def extract_and_apply_options!(args)
       options = args.last.is_a?(Hash) ? args.pop : {}
       options.assert_valid_keys :parse
 
-      args.map! { |a| self.class.from(a) } if options[:parse]
+      args.map! { |a| TagListParser.parse(a) } if options[:parse]
 
       args.flatten!
     end
 
 
+    ## DEPRECATED
+    def self.from(string)
+      ActiveRecord::Base.logger.warn <<WARNING
+ActsAsTaggableOn::TagList.from is deprecated \
+and will be removed from v4.0+, use  \
+ActsAsTaggableOn::TagListParser.parse instead
+WARNING
+      TagListParser.parse(string)
+    end
+
+
   end
 end
 

lib/acts_as_taggable_on/tag_list_parser.rb

@@ -0,0 +1,78 @@
+module ActsAsTaggableOn
+  ##
+  # Returns a new TagList using the given tag string.
+  #
+  # Example:
+  #   tag_list = ActsAsTaggableOn::TagListParser.parse("One , Two,  Three")
+  #   tag_list # ["One", "Two", "Three"]
+  module TagListParser
+    class << self
+      def parse(string)
+        string = string.join(ActsAsTaggableOn.glue) if string.respond_to?(:join)
+        TagList.new.tap do |tag_list|
+          string = string.to_s.dup
+
+
+          string.gsub!(double_quote_pattern) {
+            # Append the matched tag to the tag list
+            tag_list << Regexp.last_match[2]
+            # Return the matched delimiter ($3) to replace the matched items
+            ''
+          }
+
+          string.gsub!(single_quote_pattern) {
+            # Append the matched tag ($2) to the tag list
+            tag_list << Regexp.last_match[2]
+            # Return an empty string to replace the matched items
+            ''
+          }
+
+          # split the string by the delimiter
+          # and add to the tag_list
+          tag_list.add(string.split(Regexp.new delimiter))
+        end
+      end
+
+
+      # private
+      def delimiter
+        # Parse the quoted tags
+        d = ActsAsTaggableOn.delimiter
+        # Separate multiple delimiters by bitwise operator
+        d = d.join('|') if d.kind_of?(Array)
+        d
+      end
+
+      # (             # Tag start delimiter ($1)
+      # \A       |  # Either string start or
+      # #{delimiter}        # a delimiter
+      # )
+      # \s*"          # quote (") optionally preceded by whitespace
+      # (.*?)         # Tag ($2)
+      # "\s*          # quote (") optionally followed by whitespace
+      # (?=           # Tag end delimiter (not consumed; is zero-length lookahead)
+      # #{delimiter}\s*  |  # Either a delimiter optionally followed by whitespace or
+      # \z          # string end
+      # )
+      def double_quote_pattern
+        /(\A|#{delimiter})\s*"(.*?)"\s*(?=#{delimiter}\s*|\z)/
+      end
+
+      # (             # Tag start delimiter ($1)
+      # \A       |  # Either string start or
+      # #{delimiter}        # a delimiter
+      # )
+      # \s*'          # quote (') optionally preceded by whitespace
+      # (.*?)         # Tag ($2)
+      # '\s*          # quote (') optionally followed by whitespace
+      # (?=           # Tag end delimiter (not consumed; is zero-length lookahead)
+      # #{delimiter}\s*  | d # Either a delimiter optionally followed by whitespace or
+      # \z          # string end
+      # )
+      def single_quote_pattern
+        /(\A|#{delimiter})\s*'(.*?)'\s*(?=#{delimiter}\s*|\z)/
+      end
+
+    end
+  end
+end

lib/acts_as_taggable_on/taggable/core.rb

@@ -82,7 +82,7 @@ def grouped_column_names_for(object)
       #   User.tagged_with("awesome", "cool", :match_all => true) # Users that are tagged with just awesome and cool
       #   User.tagged_with("awesome", "cool", :owned_by => foo ) # Users that are tagged with just awesome and cool by 'foo'
       def tagged_with(tags, options = {})
-        tag_list = ActsAsTaggableOn::TagList.from(tags)
+        tag_list = ActsAsTaggableOn::TagListParser.parse(tags)
         options = options.dup
         empty_result = where('1 = 0')
 
@@ -116,7 +116,7 @@ def tagged_with(tags, options = {})
                 " AND #{ActsAsTaggableOn::Tagging.table_name}.tagger_type = #{quote_value(owned_by.class.base_class.to_s, nil)}"
           end
 
-        elsif options.delete(:any)
+        elsif any = options.delete(:any)
           # get tags, drop out if nothing returned (we need at least one)
           tags = if options.delete(:wild)
                    ActsAsTaggableOn::Tag.named_like_any(tag_list)
@@ -141,7 +141,7 @@ def tagged_with(tags, options = {})
 
           # don't need to sanitize sql, map all ids and join with OR logic
           conditions << tags.map { |t| "#{taggings_alias}.tag_id = #{quote_value(t.id, nil)}" }.join(' OR ')
-          select_clause = " #{table_name}.*" unless context and tag_types.one?
+          select_clause << " #{table_name}.*" unless context and tag_types.one?
 
           if owned_by
             tagging_join << ' AND ' +
@@ -153,7 +153,10 @@ def tagged_with(tags, options = {})
           end
 
           joins << tagging_join
-          group = "#{table_name}.#{primary_key}"
+          unless any == 'distinct'    # Fix issue #544
+            group = "#{table_name}.#{primary_key}"
+            select_clause << group
+          end
         else
           tags = ActsAsTaggableOn::Tag.named_any(tag_list)
 
@@ -183,7 +186,7 @@ def tagged_with(tags, options = {})
 
         group ||= [] # Rails interprets this as a no-op in the group() call below
         if options.delete(:order_by_matching_tag_count)
-          select_clause = "#{table_name}.*, COUNT(#{taggings_alias}.tag_id) AS #{taggings_alias}_count"
+          select_clause << "#{table_name}.*, COUNT(#{taggings_alias}.tag_id) AS #{taggings_alias}_count"
           group_columns = ActsAsTaggableOn::Utils.using_postgresql? ? grouped_column_names_for(self) : "#{table_name}.#{primary_key}"
           group = group_columns
           order_by << "#{taggings_alias}_count DESC"
@@ -203,8 +206,9 @@ def tagged_with(tags, options = {})
 
         order_by << options[:order] if options[:order].present?
 
-        select(select_clause)
-        .joins(joins.join(' '))
+        query = self
+        query = self.select(select_clause.join(',')) unless select_clause.empty?
+        query.joins(joins.join(' '))
         .where(conditions.join(' AND '))
         .group(group)
         .having(having)
@@ -259,7 +263,7 @@ def tag_list_cache_on(context)
       if instance_variable_get(variable_name)
         instance_variable_get(variable_name)
       elsif cached_tag_list_on(context) && self.class.caching_tag_list_on?(context)
-        instance_variable_set(variable_name, ActsAsTaggableOn::TagList.from(cached_tag_list_on(context)))
+        instance_variable_set(variable_name, ActsAsTaggableOn::TagListParser.parse(cached_tag_list_on(context)))
       else
         instance_variable_set(variable_name, ActsAsTaggableOn::TagList.new(tags_on(context).map(&:name)))
       end
@@ -309,7 +313,7 @@ def set_tag_list_on(context, new_list)
       variable_name = "@#{context.to_s.singularize}_list"
       process_dirty_object(context, new_list) unless custom_contexts.include?(context.to_s)
 
-      instance_variable_set(variable_name, ActsAsTaggableOn::TagList.from(new_list))
+      instance_variable_set(variable_name, ActsAsTaggableOn::TagListParser.parse(new_list))
     end
 
     def tagging_contexts

lib/acts_as_taggable_on/taggable/ownership.rb

@@ -63,7 +63,7 @@ def set_owner_tag_list_on(owner, context, new_list)
 
       cache = cached_owned_tag_list_on(context)
 
-      cache[owner] = ActsAsTaggableOn::TagList.from(new_list)
+      cache[owner] = ActsAsTaggableOn::TagListParser.parse(new_list)
     end
 
     def reload(*args)

spec/acts_as_taggable_on/acts_as_taggable_on_spec.rb

@@ -1,4 +1,4 @@
-# coding: utf-8
+# -*- encoding : utf-8 -*-
 require 'spec_helper'
 
 describe 'Acts As Taggable On' do

spec/acts_as_taggable_on/acts_as_tagger_spec.rb

@@ -1,3 +1,4 @@
+# -*- encoding : utf-8 -*-
 require 'spec_helper'
 
 describe 'acts_as_tagger' do

spec/acts_as_taggable_on/caching_spec.rb

@@ -1,3 +1,4 @@
+# -*- encoding : utf-8 -*-
 require 'spec_helper'
 
 describe 'Acts As Taggable On' do

spec/acts_as_taggable_on/related_spec.rb

@@ -1,3 +1,4 @@
+# -*- encoding : utf-8 -*-
 require 'spec_helper'
 
 describe 'Acts As Taggable On' do

spec/acts_as_taggable_on/single_table_inheritance_spec.rb

@@ -1,3 +1,4 @@
+# -*- encoding : utf-8 -*-
 require 'spec_helper'
 
 describe 'Single Table Inheritance' do

spec/acts_as_taggable_on/tag_list_parser_spec.rb

@@ -0,0 +1,46 @@
+# -*- encoding : utf-8 -*-
+require 'spec_helper'
+
+describe ActsAsTaggableOn::TagListParser do
+  it '#parse should return empty array if empty array is passed' do
+    expect(ActsAsTaggableOn::TagListParser.parse([])).to be_empty
+  end
+
+  describe 'Multiple Delimiter' do
+    before do
+      @old_delimiter = ActsAsTaggableOn.delimiter
+    end
+
+    after do
+      ActsAsTaggableOn.delimiter = @old_delimiter
+    end
+
+    it 'should separate tags by delimiters' do
+      ActsAsTaggableOn.delimiter = [',', ' ', '\|']
+      tag_list = ActsAsTaggableOn::TagListParser.parse('cool, data|I have')
+      expect(tag_list.to_s).to eq('cool, data, I, have')
+    end
+
+    it 'should escape quote' do
+      ActsAsTaggableOn.delimiter = [',', ' ', '\|']
+      tag_list = ActsAsTaggableOn::TagListParser.parse "'I have'|cool, data"
+      expect(tag_list.to_s).to eq('"I have", cool, data')
+
+      tag_list = ActsAsTaggableOn::TagListParser.parse '"I, have"|cool, data'
+      expect(tag_list.to_s).to eq('"I, have", cool, data')
+    end
+
+    it 'should work for utf8 delimiter and long delimiter' do
+      ActsAsTaggableOn.delimiter = ['，', '的', '可能是']
+      tag_list = ActsAsTaggableOn::TagListParser.parse('我的东西可能是不见了，还好有备份')
+      expect(tag_list.to_s).to eq('我， 东西， 不见了， 还好有备份')
+    end
+
+    it 'should work for multiple quoted tags' do
+      ActsAsTaggableOn.delimiter = [',']
+      tag_list = ActsAsTaggableOn::TagListParser.parse('"Ruby Monsters","eat Katzenzungen"')
+      expect(tag_list.to_s).to eq('Ruby Monsters, eat Katzenzungen')
+    end
+  end
+
+end