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 pluralization based on locale setup for any language #899

Merged
merged 2 commits into from Jul 17, 2017

Conversation

hundred
Copy link
Contributor

@hundred hundred commented Jul 10, 2017

this solves the issue with using .pluralize for languages like :de etc, that dont always add s on the end of the pluralized words

@yuki24
Copy link
Member

yuki24 commented Jul 10, 2017

Thanks so much! Overall, it looks awesome to me. I'll add a more detailed review later today.

@hundred
Copy link
Contributor Author

hundred commented Jul 13, 2017

@yuki24 did you have a chance to look at the PR yet? :)

@@ -266,6 +268,132 @@ def to_s
I18n.backend.reload!
end
end

sub_test_case 'with any other locale' do
sub_test_case ':de' do
Copy link
Member

@yuki24 yuki24 Jul 17, 2017

Choose a reason for hiding this comment

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

I wonder if this entire sub test case for :de is necessary when there is a test case for :fr. Are there any tests that fail without your change?

ActiveSupport::Inflector.inflections(:fr) do |inflect|
inflect.plural(/$/, 's')
inflect.singular(/s$/, '')
end
Copy link
Member

Choose a reason for hiding this comment

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

This looks great 👍🏻

)
assert_equal 'Displaying <b>1</b> utilisateur', view.page_entries_info(users, entry_name: 'utilisateur')
ensure
I18n.backend.reload!
Copy link
Member

@yuki24 yuki24 Jul 17, 2017

Choose a reason for hiding this comment

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

Instead of using begin...ensure...end, I think it makes more sense to do this with setup do ... end and call a I18n.backend.reload! in teardown do ... end.

@hundred
Copy link
Contributor Author

hundred commented Jul 17, 2017

@yuki24 thanks for review, moved those now :)
the point of having :de in test was to show/make sure that pluralize wont auto-add an s to plural for that language,
if the change itself in the helper is ever questioned/removed, this will be the only test that fails as :fr uses the same logic as english for pluralization by mostly just adding an s in the end of the word
let me know if this makes sense :) ?

@yuki24
Copy link
Member

yuki24 commented Jul 17, 2017

the point of having :de in test was to show/make sure that pluralize wont auto-add an s to plural for that language

Thanks for the clarification. That makes a lot of sense.

Thanks for your contribution!

@yuki24 yuki24 merged commit 2d22392 into kaminari:master Jul 17, 2017
@zerocool4u2
Copy link

After some digging I notice that this works on 1.0.1 out of the box if you have inflectors for each language you wanted, but there is a problem on entry_name, but don't really know what exactly is...

I'm currently overwriting that method and it seems to work as expected:

module Kaminari
	module ActiveRecordRelationMethods
		extend Kaminari::ActiveRecordRelationMethods

		def self.included(base)
			base.send :extend, Kaminari::ActiveRecordRelationMethods
		end

		def entry_name(options = {})
			model_name.human.pluralize(options[:count])
		end
	end
end

if options is nil or :count is nil, it would be just one item by default on pluralize, same with a single item

@yuki24
Copy link
Member

yuki24 commented Sep 28, 2017

@zerocool4u2 I don't think I understand your issue. Please file a new issue with reproduction steps or a test case that fails.

@zerocool4u2
Copy link

I mean that this change

entry_name.pluralize(collection.size, I18n.locale)

Doesn't make any difference, pluralize already takes the current I18n.locale and if you have inflectors for pluralization it would pluralize any word out of the box, rather it would be more useful to modify entry_name so you get pluralization in local language without passing a entry_name variable to page_entries_info and leave entry_name option for when you want to use a different name in a determinate case.

Now this other method isn't working because default is just for when you don't have a translation http://guides.rubyonrails.org/i18n.html#defaults

def entry_name(options = {})
      default = options[:count] == 1 ? model_name.human : model_name.human.pluralize
      model_name.human(options.reverse_merge(default: default))
end

Maybe this is intended, but my suggestion would be to use something like this

def entry_name(options = {})
      model_name.human.pluralize(options[:count])
end

Hope that this clarifies what I was saying @yuki24 , basically I want this feature, but this isn't the solution :(

@yuki24
Copy link
Member

yuki24 commented Sep 28, 2017

@zerocool4u2 Again, please file a new issue with reproduction steps or a test case that fails and what you'd expect to see.

@kaminari kaminari locked and limited conversation to collaborators Sep 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants