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

plural? and singular? methods for String #6802

Closed
wants to merge 1 commit into from

Conversation

butzopower
Copy link

I wanted to know if a string was plural or singular, so I added string extensions.

# 'post'.singular? # => true
# 'posts'.singular? # => false
# 'sheep'.singular? # => true

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should delete the empty lines.

@dmathieu
Copy link
Contributor

Actually, it's not that trivial. We could expect that any plural value is not singular.
Though, it you add a test checking that, you'll see that some inflected values will be considered both as singular and plural.

@butzopower
Copy link
Author

I think that's okay though, isn't it? Some words are both the plural and
singular form. I even explicitly count for it in the tests by asserting
fish returns true for both #plural? and #singular?.
On Jun 20, 2012 1:41 PM, "Damien Mathieu" <
reply@reply.github.com>
wrote:

Actually, it's not that trivial. We could expect that any plural value is
not singular.
Though, it you add a test checking that, you'll see that some inflected
values will be considered both as singular and plural.


Reply to this email directly or view it on GitHub:
#6802 (comment)

@butzopower
Copy link
Author

Sorry, reading my original description, I see where this may have been a bit confusing. I didn't mean I wanted to check if a word was exclusively either singular or was plural, having it return true in both cases for words with the same form would be okay.

@chancancode
Copy link
Member

I looked at this I thought, "Hey, I tried to do this before and I thought this is not supposed to work...". In case someone else is wondering about the same thing, here's why:

In Rails <= 3.2:

>> "address".singularize
=> "addres"

For the longest time, (given the "frozen" status of the inflector) I thought this is how it's supposed to be (i.e. singularize is not idempotent), hence what @butzopower proposed would not work.

It turns out this is actually a bug and it was fixed in master in #4719. Based on the test cases in activesupport/test/inflector_test.rb, it does appear the infectors are supposed to be idempotent - at least that's the intention. Therefore what @butzopower proposed is supposed to work on master.

@chancancode
Copy link
Member

However, we need to make sure people who uses this understands that this is only as reliable as the infector itself. Edge cases that would normally break the infector would break this also. (Couldn't think of any off the top of my head, most infector "bugs" are around x.pluralize.singularize != x, or the other way around.)

The implications are:

  1. If this doesn't work for you, you would have to fix it yourself by adding your own infector rule. [Unless your problem affects > 50K users. See Added irregular zombie inflection, so zombies != zomby #2457 ;) ]
  2. This really isn't meant for general purpose use cases (validating user input), because it is not guaranteed that it would work correctly on arbitrary input. Just like the infector itself, it's really designed for things like Rails model names - a limited set of things in your code that you have control over, so that when things do not work the way you expected you can add custom inflector rules to address the problem.

Edit:

Finally I have an example "edge case" for you:

>> 'bonus'.singularize == 'bonus'
=> false

This proves my point that people should not expect this to work for arbitrary input. If you try, you definitely will run into problems very quickly.

end

assert(!"search".plural?)
assert("fish".plural?)
Copy link
Member

Choose a reason for hiding this comment

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

  1. Why these extra assertions? They should be covered in SingularToPlural already.
  2. Consider mirroring the test cases in https://github.com/rails/rails/blob/master/activesupport/test/inflector_test.rb#L45-71, so that when each of the words are tested separately in their own test case. i.e. When one or more of them failed, the rest will still be tested instead of skipped.

Edit: Nvm my point #2. I was looking at the wrong file (inflector_test.rb). What you are doing here seems to be the norm for this file, so that's probably fine. The extra "search" and "fish" should still be removed though.

Copy link
Author

Choose a reason for hiding this comment

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

I realized later that the fish test was redundant.

For "search" I was trying to assert that the method returns false when the string is not the plural form, just to have a negative assertion so that I couldn't make the test pass by just always returning true. I could mock the inflector call to return a different string, if that's preferred.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that makes sense.

@butzopower
Copy link
Author

Is there anything else needed on this? I agree that people should not be using it in validations or really any place that has to do with managing persistance. I think some useful cases would be in helpers / presenters. I can up the documentation to make this more clear, but I think there's already a pretty implicit heed to be cautious around the inflector in general.

The issue with bonus seems like a separate issue around inflections, and these methods would return valid results if it were fixed.

end

assert(!"searches".singular?)
end

Choose a reason for hiding this comment

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

I think these assertions would look better without (), as in the inflector, wdyt?

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good.

@carlosantoniodasilva
Copy link
Member

I think it's a fine addition since we have pluralize and singularize, but I'd like to hear @fxn's feedback first. In any case, some guide updates and a changelog entry would be great :). Thanks!

@butzopower
Copy link
Author

I assumed this goes in the in the 4.0 changelog, that was right, right?

@chancancode
Copy link
Member

Since you branched off master, yes it should go into 4.0 changelog. You might also want to get ready for a clean merge by rebasing against master and squashing commits. Also, take a look at the new instructions for formatting the commit message at http://guides.rubyonrails.org/contributing_to_ruby_on_rails.html#commit-your-changes :)

@butzopower
Copy link
Author

Alright, squashed and rebased on top of latest rails/master. Lemme know if there's anything else I need to do.

@butzopower
Copy link
Author

/cc @fxn We'd like to hear your feedback.

@frodsan
Copy link
Contributor

frodsan commented Oct 27, 2012

👍 This needs a rebase.

@ghost ghost assigned fxn Oct 27, 2012
Methods to determine if a String is plural or singular by comparing it
to its inflected form.  The methods are defined in the Inflector itself
and then called from methods in the String core extension.
@butzopower
Copy link
Author

@frodsan Done.

@rafaelfranca
Copy link
Member

I'm not a fan of this feature because I don't think a lot of applications will get advantages of it. Also the implementation is so simple that you can add in your application.

The inflector can return some false positive results and this feature will break, leading more pull requests "fixing" the inflectors.

That said I'm closing this one.

Thank you so much for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants