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

Add support for cyrillic search #873

Merged
merged 1 commit into from
May 9, 2017
Merged

Conversation

dminchev
Copy link
Contributor

@dminchev dminchev commented May 8, 2017

This update adds support for cyrillic and other language search and doesn't change the original search behaviour.

@@ -30,7 +30,7 @@ def query
end

def search_terms
["%#{term.downcase}%"] * search_attributes.count
["%#{term.mb_chars.downcase.to_s}%"] * search_attributes.count

Choose a reason for hiding this comment

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

Redundant use of Object#to_s in interpolation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at this and yes, you shouldn't need #to_s since you're interpolating and the return value of String#downcase is a string.

@nickcharlton nickcharlton added this to the v0.8.0 milestone May 8, 2017
@nickcharlton
Copy link
Member

Hi @dminchev! I think this is good to merge, if you'd be able to address Hound's comments?

@dminchev
Copy link
Contributor Author

dminchev commented May 9, 2017

Hi @nickcharlton, thanks for the feedback! I'll update it and let you know.

@dminchev
Copy link
Contributor Author

dminchev commented May 9, 2017

@nickcharlton I've updated the redundant usage and the tests are passing successfully local. However on circleci there is failure which I think is not related to the changes. I'm not able to rerun the circleci build. Can you please check it?

@nickcharlton
Copy link
Member

Oh, that's interesting. We'd thought that #872 had fixed this. What if you rebase your branch on top of current master? Hopefully it should go away.

@@ -1,4 +1,7 @@
#{$all-buttons},
button,

Choose a reason for hiding this comment

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

Avoid qualifying attribute selectors with an element.

@dminchev
Copy link
Contributor Author

dminchev commented May 9, 2017

I've updated my master branch with the upstream master branch however there is one new violation found by hound.

@nickcharlton
Copy link
Member

Oh, it looks like you've picked up a bunch of previous commits.

This sometimes happens if the history is slightly older and git can't tell. I'd expect to see just your "Add support for cyrillic search", "Fix redundant use of Object#to_s in interpolation." commits. Would you mind rebasing again?

@dminchev
Copy link
Contributor Author

dminchev commented May 9, 2017

All should be ok now

@nickcharlton
Copy link
Member

Perfect! Thanks. Merging.

@nickcharlton nickcharlton merged commit 9dc81e5 into thoughtbot:master May 9, 2017
@dminchev
Copy link
Contributor Author

dminchev commented May 9, 2017

Great, thanks!

iarie pushed a commit to iarie/administrate that referenced this pull request Jun 17, 2017
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.

4 participants