Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rails 5.2 support, drop Rails 4.2 support. #887

Merged
merged 9 commits into from
Jun 6, 2018
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ env:
- DB=postgresql

gemfile:
- gemfiles/activerecord_5.2.gemfile
- gemfiles/activerecord_5.1.gemfile
- gemfiles/activerecord_5.0.gemfile
- gemfiles/activerecord_4.2.gemfile

sudo: false

Expand Down
8 changes: 4 additions & 4 deletions Appraisals
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
appraise 'activerecord-5.2' do
gem 'activerecord', '5.2.0'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not using a version like ~> 5.2.0?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, why specifically 5.2 and not 5.0? In the Gemspec you specified 5.0, I suggest we stick with that version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get that. This is PR to add support for 5.2, why would I specify 5.0 in 5.2 appraisal?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we're dropping support for 4.2, it does not mean that we drop support for 5.0 I guess, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, that's why Appraisal file has definitions for 5.0, 5.1 and now for 5.2

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gem 'activerecord', '~> 5.2.0' will install the latest minor version of the 5.2 serie

end

appraise 'activerecord-5.1' do
gem 'activerecord', "~> 5.1.1"
end

appraise 'activerecord-5.0' do
gem 'activerecord', "~> 5.0.3"
end

appraise "activerecord-4.2" do
gem "activerecord", "~> 4.2.8"
end
2 changes: 1 addition & 1 deletion acts-as-taggable-on.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ Gem::Specification.new do |gem|
gem.post_install_message = File.read('UPGRADING.md')
end

gem.add_runtime_dependency 'activerecord', ['>= 4.2.8']
gem.add_runtime_dependency 'activerecord', ['>= 5.0']
Copy link
Collaborator

@seuros seuros Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gem.add_runtime_dependency 'activerecord', ['~> 5.0.0'] To avoid install in 6.*


gem.add_development_dependency 'sqlite3'
gem.add_development_dependency 'mysql2', '~> 0.3'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@

source "https://rubygems.org"

gem "activerecord", "~> 4.2.8"
gem "pg", "~> 0.18"
gem "mysql2", "< 0.5"
gem "activerecord", "5.2.0"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment, why not sticking with 5.0?


group :local_development do
gem "guard"
Expand Down
1 change: 0 additions & 1 deletion lib/acts_as_taggable_on/taggable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ def self.taggable?
include Cache
include Ownership
include Related
include Dirty
end
end
end
2 changes: 1 addition & 1 deletion lib/acts_as_taggable_on/taggable/collection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ module CalculationMethods
# See https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/calculations.rb#L38
def count(column_name = :all, options = {})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the whole method can be removed now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried - some of tests fail, especially sqlite ones. Let's not risk it this time :)

# https://github.com/rails/rails/commit/da9b5d4a8435b744fcf278fffd6d7f1e36d4a4f2
ActsAsTaggableOn::Utils.active_record5? ? super(column_name) : super(column_name, options)
super(column_name)
end
end
end
Expand Down
45 changes: 23 additions & 22 deletions lib/acts_as_taggable_on/taggable/core.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

module ActsAsTaggableOn::Taggable
module Core

def self.included(base)
base.extend ActsAsTaggableOn::Taggable::Core::ClassMethods

Expand All @@ -28,12 +29,16 @@ def initialize_acts_as_taggable_on_core
has_many context_taggings, -> { includes(:tag).order(taggings_order).where(context: tags_type) },
as: :taggable,
class_name: 'ActsAsTaggableOn::Tagging',
dependent: :destroy
dependent: :destroy,
after_add: :dirtify_tag_list,
after_remove: :dirtify_tag_list

has_many context_tags, -> { order(taggings_order) },
class_name: 'ActsAsTaggableOn::Tag',
through: context_taggings,
source: :tag

attribute "#{tags_type.singularize}_list".to_sym, ActiveModel::Type::Value.new

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line makes taggable_obj.attribute_names changed. Specs failed in the following case, using json.extract! @item, *@item.attribute_names as response.

I saw you comment this PR will be released as the version 6.0.0 and allow breaking changes, but the impact of this change, if you don't expect, is very big.

What do you think about this case?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand the issue. Could you provide a failing spec?

Though I think if it's 6.0.0 release, then breaking changes are totally ok, just repo owner has to update readme before reasling this new version.

Copy link

@ebihara99999 ebihara99999 Jun 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failing is request spec with the rspec-json_matcher gem. I copy and modify some from the real codes and output of running rspec:

# request spec
it { is_expected.to be_json_as [product: expected_product_response] }
# output
expected to match:

  {
    :product => {
      :id          => Integer < Numeric,
      :user_id     => Integer < Numeric,
      :product_id  => Integer < Numeric,
      :created_at  => String < Object,
      :updated_at  => String < Object,
    }
  }

actual:
  {
    :product => {
      "id"          => 2,
      "user_id"     => 11,
      "product_id"  => 216,
      "tag_list"    => [] # This is the difference
      "created_at"  => "2018-06-05T17:36:33+09:00",
      "updated_at"  => "2018-06-05T17:36:33+09:00"
    }
  }


reason: product "tag_list"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case mentioned above is with the rspec-json_matcher gem, but if not using the rspec-json_matcher gem, the test suites testing api response would all fail because of the change.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion this change is harmful #1064

end

taggable_mixin.class_eval <<-RUBY, __FILE__, __LINE__ + 1
Expand All @@ -42,12 +47,24 @@ def #{tag_type}_list
end

def #{tag_type}_list=(new_tags)
parsed_new_list = ActsAsTaggableOn.default_parser.new(new_tags).parse

if self.class.preserve_tag_order? || parsed_new_list.sort != #{tag_type}_list.sort
set_attribute_was('#{tag_type}_list', #{tag_type}_list)
write_attribute("#{tag_type}_list", parsed_new_list)
end

set_tag_list_on('#{tags_type}', new_tags)
end

def all_#{tags_type}_list
all_tags_list_on('#{tags_type}')
end

private
def dirtify_tag_list(tagging)
attribute_will_change! tagging.context.singularize+"_list"
end
RUBY
end
end
Expand Down Expand Up @@ -161,7 +178,7 @@ def all_tags_on(context)

if ActsAsTaggableOn::Utils.using_postgresql?
group_columns = grouped_column_names_for(ActsAsTaggableOn::Tag)
scope.order("max(#{tagging_table_name}.created_at)").group(group_columns)
scope.order(Arel.sql("max(#{tagging_table_name}.created_at)")).group(group_columns)
else
scope.group("#{ActsAsTaggableOn::Tag.table_name}.#{ActsAsTaggableOn::Tag.primary_key}")
end.to_a
Expand All @@ -181,33 +198,16 @@ def set_tag_list_on(context, new_list)
add_custom_context(context)

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.default_parser.new(new_list).parse)
parsed_new_list = ActsAsTaggableOn.default_parser.new(new_list).parse

instance_variable_set(variable_name, parsed_new_list)
end

def tagging_contexts
self.class.tag_types.map(&:to_s) + custom_contexts
end

def process_dirty_object(context, new_list)
value = new_list.is_a?(Array) ? ActsAsTaggableOn::TagList.new(new_list) : new_list
attrib = "#{context.to_s.singularize}_list"

if changed_attributes.include?(attrib)
# The attribute already has an unsaved change.
old = changed_attributes[attrib]
@changed_attributes.delete(attrib) if old.to_s == value.to_s
else
old = tag_list_on(context)
if self.class.preserve_tag_order
@changed_attributes[attrib] = old if old.to_s != value.to_s
else
@changed_attributes[attrib] = old.to_s if old.sort != ActsAsTaggableOn.default_parser.new(value).parse.sort
end
end
end

def reload(*args)
self.class.tag_types.each do |context|
instance_variable_set("@#{context.to_s.singularize}_list", nil)
Expand Down Expand Up @@ -315,3 +315,4 @@ def find_or_create_tags_from_list_with_context(tag_list, _context)
end
end
end

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one was fine actually, It's good to have a blank line at the end of files :). Sorry, I should have been more specific than a 👍
https://stackoverflow.com/questions/2287967/why-is-it-recommended-to-have-empty-line-in-the-end-of-file

36 changes: 0 additions & 36 deletions lib/acts_as_taggable_on/taggable/dirty.rb

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ class AnyTagsQuery < QueryBase
def build
taggable_model.select(all_fields)
.where(model_has_at_least_one_tag)
.order(order_conditions)
.order(Arel.sql(order_conditions))
.readonly(false)
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def tags_match_type

def escaped_tag(tag)
tag = tag.downcase unless ActsAsTaggableOn.strict_case_match
ActsAsTaggableOn::Utils.escape_like(tag)
tag.gsub(/[!%_]/) { |x| '!' + x }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should be crazily spec'ed :).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just a rollback of cc7a2c0, nothing I wrote for this PR. That commit breaks builds in master branch.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can use ActsAsTaggableOn::Utils.escape_like(tag)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to repeat myself, but this change is unrelated to the scope of this PR, as it barely rollbacks commit that broke the build here and in master branch

end

def adjust_taggings_alias(taggings_alias)
Expand Down
2 changes: 1 addition & 1 deletion lib/acts_as_taggable_on/tagging.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class Tagging < ::ActiveRecord::Base #:nodoc:
belongs_to :tag, class_name: '::ActsAsTaggableOn::Tag', counter_cache: ActsAsTaggableOn.tags_counter
belongs_to :taggable, polymorphic: true

belongs_to :tagger, {polymorphic: true}.tap {|o| o.merge!(optional: true) if ActsAsTaggableOn::Utils.active_record5? }
belongs_to :tagger, {polymorphic: true}.tap {|o| o.merge!(optional: true) }

scope :owned_by, ->(owner) { where(tagger: owner) }
scope :not_owned, -> { where(tagger_id: nil, tagger_type: nil) }
Expand Down
10 changes: 1 addition & 9 deletions lib/acts_as_taggable_on/utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,13 @@ def sha_prefix(string)
Digest::SHA1.hexdigest(string)[0..6]
end

def active_record5?
::ActiveRecord::VERSION::MAJOR == 5
end

def like_operator
using_postgresql? ? 'ILIKE' : 'LIKE'
end

# escape _ and % characters in strings, since these are wildcards in SQL.
def escape_like(str)
str.gsub(/[!%_]/) { |x| escape_replacement + x }
end

def escape_replacement
using_postgresql? ? '\\' : '!'
str.gsub(/[!%_]/) { |x| '!' + x }
end
end
end
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# -*- encoding : utf-8 -*-
require 'spec_helper'

describe ActsAsTaggableOn::Taggable::Dirty do
describe 'Dirty behavior of taggable objects' do
context 'with un-contexted tags' do
before(:each) do
@taggable = TaggableModel.create(tag_list: 'awesome, epic')
Expand All @@ -14,19 +14,27 @@
end

it 'should show changes of dirty object' do
expect(@taggable.changes).to eq({'tag_list' => ['awesome, epic', ['one']]})
expect(@taggable.changes).to eq({'tag_list' => [['awesome', 'epic'], ['one']]})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️

This is a breaking change.

Either rollback your change or we have to communicate crazy about that. Our own internal code depends on the previous way it was formatted, and I bet a bunch of projects have the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Erowlin this PR is tagged for release 6.0.0, which is supposed to have breaking changes, no?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I guess we're fine!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a major breaking change that we should avoid.
People expect to have tags in the root of the array not inside another array.
Can we try to avoid this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can not promise I will have any time to invest into it in next days/weeks. Even though new behaviour seems much more logical to me, because previously it was a change from string to array, and now it's change from array to array ( no akward change of data type).

end

it 'flags tag_list as changed' do
expect(@taggable.tag_list_changed?).to be_truthy
it 'should show changes of freshly initialized dirty object' do
taggable = TaggableModel.find(@taggable.id)
taggable.tag_list = 'one'
expect(taggable.changes).to eq({'tag_list' => [['awesome', 'epic'], ['one']]})
end

if Rails.version >= "5.1"
it 'flags tag_list as changed' do
expect(@taggable.will_save_change_to_tag_list?).to be_truthy
end
end

it 'preserves original value' do
expect(@taggable.tag_list_was).to eq('awesome, epic')
expect(@taggable.tag_list_was).to eq(['awesome', 'epic'])
end

it 'shows what the change was' do
expect(@taggable.tag_list_change).to eq(['awesome, epic', ['one']])
expect(@taggable.tag_list_change).to eq([['awesome', 'epic'], ['one']])
end

context 'without order' do
Expand Down Expand Up @@ -90,23 +98,19 @@
end

it 'should show changes of dirty object' do
expect(@taggable.changes).to eq({'language_list' => ['awesome, epic', ['one']]})
expect(@taggable.changes).to eq({'language_list' => [['awesome', 'epic'], ['one']]})
end

it 'flags language_list as changed' do
expect(@taggable.language_list_changed?).to be_truthy
end

it 'preserves original value' do
expect(@taggable.language_list_was).to eq('awesome, epic')
expect(@taggable.language_list_was).to eq(['awesome', 'epic'])
end

it 'shows what the change was' do
expect(@taggable.language_list_change).to eq(['awesome, epic', ['one']])
end

it 'shows what the changes were' do
expect(@taggable.language_list_changes).to eq(['awesome, epic', ['one']])
expect(@taggable.language_list_change).to eq([['awesome', 'epic'], ['one']])
end
end

Expand All @@ -123,5 +127,16 @@
expect(@taggable.changes).to be_empty
end
end

context 'when language_list changed by association' do
let(:tag) { ActsAsTaggableOn::Tag.new(name: 'one') }

it 'flags language_list as changed' do
expect(@taggable.changes).to be_empty
@taggable.languages << tag
expect(@taggable.language_list_changed?).to be_truthy
end
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Un-needed new line

end
end
12 changes: 0 additions & 12 deletions spec/acts_as_taggable_on/utils_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,4 @@
expect(ActsAsTaggableOn::Utils.sha_prefix('puppies')).not_to eq(ActsAsTaggableOn::Utils.sha_prefix('kittens'))
end
end

describe '#escape_replacement' do
it 'should return ! when the adapter is not PostgreSQL' do
allow(ActsAsTaggableOn::Utils.connection).to receive(:adapter_name) { 'MySQL' }
expect(ActsAsTaggableOn::Utils.escape_replacement).to eq('!')
end

it 'should return \\ when the adapter is PostgreSQL' do
allow(ActsAsTaggableOn::Utils.connection).to receive(:adapter_name) { 'PostgreSQL' }
expect(ActsAsTaggableOn::Utils.escape_replacement).to eq('\\')
end
end
end