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

Use database's lower function for case-insensitive match #498

Merged
merged 2 commits into from Mar 21, 2014

Conversation

leo-souza
Copy link
Contributor

This PR is for the same problem as PR #492 but with a diferent solution, since it was breaking the build.
It's similar to PR #472 and relates to issue #464

@leo-souza
Copy link
Contributor Author

I ran tests on ruby 1.9.3 and it all went alright.

(master=)~/acts-as-taggable-on$ DB='sqlite3' rspec spec/acts_as_taggable_on/acts_as_taggable_on_spec.rb spec/acts_as_taggable_on/acts_as_tagger_spec.rb spec/acts_as_taggable_on/caching_spec.rb spec/acts_as_taggable_on/related_spec.rb spec/acts_as_taggable_on/single_table_inheritance_spec.rb spec/acts_as_taggable_on/tag_list_spec.rb spec/acts_as_taggable_on/tag_spec.rb spec/acts_as_taggable_on/taggable_spec.rb spec/acts_as_taggable_on/tagger_spec.rb spec/acts_as_taggable_on/tagging_spec.rb spec/acts_as_taggable_on/tags_helper_spec.rb spec/acts_as_taggable_on/utils_spec.rb

Warning: you should require 'minitest/autorun' instead.
Warning: or add 'gem "minitest"' before 'require "minitest/autorun"'
From:
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:247:in `block in require'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:232:in `load_dependency'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:247:in `require'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/1.9.1/test/unit/assertions.rb:1:in `<top (required)>'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:247:in `block in require'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:232:in `load_dependency'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:247:in `require'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/rspec-rails-2.13.0/lib/rspec/rails/adapters.rb:3:in `<top (required)>'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:247:in `block in require'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:232:in `load_dependency'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:247:in `require'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/ammeter-0.2.9/lib/ammeter/rspec/generator/example/generator_example_group.rb:3:in `<top (required)>'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:247:in `block in require'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:232:in `load_dependency'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:247:in `require'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/ammeter-0.2.9/lib/ammeter/rspec/generator/example.rb:2:in `<top (required)>'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:247:in `block in require'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:232:in `load_dependency'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:247:in `require'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/ammeter-0.2.9/lib/ammeter/init.rb:1:in `<top (required)>'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:247:in `block in require'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:232:in `load_dependency'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/activesupport-4.1.0.rc1/lib/active_support/dependencies.rb:247:in `require'
  /home/leo/acts-as-taggable-on/spec/spec_helper.rb:7:in `<top (required)>'
  /home/leo/acts-as-taggable-on/spec/acts_as_taggable_on/acts_as_taggable_on_spec.rb:2:in `<top (required)>'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/rspec-core-2.13.1/lib/rspec/core/configuration.rb:819:in `load'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/rspec-core-2.13.1/lib/rspec/core/configuration.rb:819:in `block in load_spec_files'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/rspec-core-2.13.1/lib/rspec/core/configuration.rb:819:in `each'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/rspec-core-2.13.1/lib/rspec/core/configuration.rb:819:in `load_spec_files'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/rspec-core-2.13.1/lib/rspec/core/command_line.rb:22:in `run'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/rspec-core-2.13.1/lib/rspec/core/runner.rb:80:in `run'
  /home/leo/.rbenv/versions/1.9.3-p484/lib/ruby/gems/1.9.1/gems/rspec-core-2.13.1/lib/rspec/core/runner.rb:17:in `block in autorun'
DEPRECATION WARNING: Passing a string to ActiveRecord::Base.establish_connection for a configuration lookup is deprecated, please pass a symbol (:sqlite3) instead. (called from <top (required)> at /home/leo/acts-as-taggable-on/spec/spec_helper.rb:31)
....................................................................................................................................log writing failed. "\xE3" from ASCII-8BIT to UTF-8
........................................................................................

Finished in 9 minutes 17.05 seconds
220 examples, 0 failures

(master=)~/acts-as-taggable-on$ rails -v
Rails 4.1.0.rc1

@seuros
Copy link
Collaborator

seuros commented Mar 19, 2014

It was failing in rails 3.x.

Where is travis when we need it ? :)

@leo-souza
Copy link
Contributor Author

what's the build #? the one's I've seen were failing in rails 4.0, 4.1 and edge (428, 429, 430)

@seuros
Copy link
Collaborator

seuros commented Mar 19, 2014

My mistake. I confused it with another PR.
I will merge this one once Travis pass the tests.

@leo-souza
Copy link
Contributor Author

Encoding is hell!
Tests are failing and it seems they're related to this rails/rails#13816
invalid byte sequence in UTF-8

@seuros
Copy link
Collaborator

seuros commented Mar 20, 2014

I think this one will pass ! Thank you @leo-souza

@leo-souza
Copy link
Contributor Author

I also added a note on the ICU extension and changed changelog for this PR since this one overwrote that one. is it ok?

@seuros
Copy link
Collaborator

seuros commented Mar 20, 2014

Perfect. 👍

I need to add tests for Arabic,Chinese and Japanese , since these language don't have upcase/downcase.

@bf4
Copy link
Collaborator

bf4 commented Mar 20, 2014

@seuros I can write one for hebrew. What about emoji?

עברית

العربية

日本の

中国的

@seuros
Copy link
Collaborator

seuros commented Mar 20, 2014

Go for it 👍

I'm not sure about emoji . But if they pass, why not ?

Are you going to add it to this PR ? @leo-souza

@leo-souza
Copy link
Contributor Author

what do you mean? tests for languages with no case?

@seuros
Copy link
Collaborator

seuros commented Mar 20, 2014

I mean we should add it to : "should not care about case for unicode names"

@bf4
Copy link
Collaborator

bf4 commented Mar 20, 2014

One option is to offer an adapter for case-insensitive comparison. By default we do 'x', but if you implement 'to_lowercase', you can use that.

@leo-souza
Copy link
Contributor Author

Maybe it should go in another test, because that one only runs in non-sqlite databases. I was thinking something like this

it "should be able to create and find tags in languages without capitalization"
    ActsAsTaggableOn.strict_case_match = false
    chihiro = TaggableModel.create(:name => "Chihiro", :tag_list => "日本の")
    salim = TaggableModel.create(:name => "Salim", :tag_list => "עברית")
    ieie = TaggableModel.create(:name => "Ieie", :tag_list => "中国的")
    yasser = TaggableModel.create(:name => "Yasser", :tag_list => "العربية")
    emo = TaggableModel.create(:name => "Emo", :tag_list => "✏")

    TaggableModel.tagged_with("日本の").to_a.size.should == 1
    TaggableModel.tagged_with("עברית").to_a.size.should == 1
    TaggableModel.tagged_with("中国的").to_a.size.should == 1
    TaggableModel.tagged_with("العربية").to_a.size.should == 1
    TaggableModel.tagged_with("✏").to_a.size.should == 1
  end

@seuros
Copy link
Collaborator

seuros commented Mar 20, 2014

👍

@leo-souza
Copy link
Contributor Author

I saw a green light at the end of the tunnel... but it's gone

seuros added a commit that referenced this pull request Mar 21, 2014
Use database's lower function for case-insensitive match
@seuros seuros merged commit 1db9880 into mbleigh:master Mar 21, 2014
@seuros
Copy link
Collaborator

seuros commented Mar 21, 2014

Don't worry, i had just to restart the job. Everything is fine now. Thank you for your work.

@leo-souza
Copy link
Contributor Author

No problem, I'm glad to contribute

@sintro
Copy link

sintro commented Apr 26, 2016

As I described here #464 (comment), the problem is still exists, and it depends on the database adapter and locale settings. In that case, all the code, which includes databases lower() functions will not work for Postgres, if the database uses LC_COLLATE locale not corresponding to tag's language. It is in many cases more handy to have the LC_COLLATE using "C" locale.
May be, there are some other way to reach case-insensetive matching in Postgres without depending on locale settings?

tekniklr pushed a commit to tekniklr/acts-as-taggable-on that referenced this pull request Mar 19, 2021
Use database's lower function for case-insensitive match
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants