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

Adds `not_in?` onto Object #25914

Merged
merged 1 commit into from Jul 22, 2016

Conversation

Projects
None yet
10 participants
@jmccartie
Contributor

jmccartie commented Jul 21, 2016

Introduce not_in? on Object.

As an opposite method for in?, not_in? provides equivalent support for exclusion. This turns this:

    [1,2].exclude?(user_id)

...into this:

    user_id.not_in?([1,2])
@rails-bot

This comment has been minimized.

rails-bot commented Jul 21, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @matthewd (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jul 21, 2016

Thank you for the pull request but we avoid to add new core extensions in Active Support unless they are going to be used in the framework or in a large number of applications.

In this specific case, both exclude and !in? can be used.

@jmccartie

This comment has been minimized.

Contributor

jmccartie commented Jul 21, 2016

@rafaelfranca in? is a convenience method. 100%. The purpose of this is to reduce the cognitive load with !include? (which is what in? already does for include?)

We have include? (Ruby core) and exclude? (Rails). And we have in? (Rails), but no not_in?. Why not have an opposite method to complete the pairs? This was odd to me and probably presents confusion for other Rails devs.

Why not make lives a little better for Rails devs with this simple patch?

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jul 21, 2016

Because in my opinion the core_ext of Active Support is already huge and I'd love to have things in Ruby itself not Rails.

I'm fine with adding this to Rails, but it should be proposed to ruby too. If you believe it is that useful, it should be in the language.

@rafaelfranca rafaelfranca reopened this Jul 21, 2016

@jmccartie

This comment has been minimized.

Contributor

jmccartie commented Jul 21, 2016

@rafaelfranca Is that what you're planning to do with in? and exclude?

@jmccartie

This comment has been minimized.

Contributor

jmccartie commented Jul 21, 2016

@rafaelfranca

I'm fine with adding this to Rails, but it should be proposed to ruby too. If you believe it is that useful, it should be in the language.

Seems like a reasonable compromise. I agree it should be in core. I can tackle that later and then reverse this?

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jul 21, 2016

We are taking that direction, yes. Object#itself was implemented first in Rails and later added to Ruby. Enumerable#sum was implemented in Rails and later added to Ruby. We are proposing String#blank? to Ruby too.

We never proposed in? and exclude? but that could be proposed along with not_in?. I guess I'm going to merge this and propose those later, unless you want to do.

@rafaelfranca

View changes

activesupport/test/core_ext/object/exclusion_test.rb Outdated
def test_no_method_catching
assert_raise(ArgumentError) { 1.not_in?(1) }
end

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 21, 2016

Member

✂️ this blank line.

@rafaelfranca

View changes

activesupport/lib/active_support/core_ext/object/exclusion.rb Outdated
rescue NoMethodError
raise ArgumentError.new("The parameter passed to #not_in? must respond to #include?")
end

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 21, 2016

Member

✂️ this blank line.

@rafaelfranca

View changes

activesupport/test/core_ext/object/exclusion_test.rb Outdated
class NotInTest < ActiveSupport::TestCase
def test_not_in_array
assert 1.not_in?([2, 3])
assert !2.not_in?([1,2])

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 21, 2016

Member

assert_not where you are using !

This comment has been minimized.

@jmccartie

jmccartie Jul 21, 2016

Contributor

I copied the tests verbatim from inclusion_test.rb. Want me to change those? Or keep this atomic?

This comment has been minimized.

@rafaelfranca

rafaelfranca Jul 21, 2016

Member

Only change the new tests added. We don't retroactively change styles, but we use the new style in new lines.

@jmccartie jmccartie force-pushed the jmccartie:jm/not_in branch Jul 21, 2016

@jmccartie jmccartie force-pushed the jmccartie:jm/not_in branch to 4db6ac2 Jul 21, 2016

@jmccartie

This comment has been minimized.

Contributor

jmccartie commented Jul 21, 2016

@rafaelfranca

I guess I'm going to merge this and propose those later, unless you want to do.

I don't mind doing it, but you probably have more experience proposing to Ruby core than I do. Happy to help if you need me.

@jmccartie

This comment has been minimized.

Contributor

jmccartie commented Jul 21, 2016

@rafaelfranca Not sure what's up with those Travis failures. They look systemic. Any ideas?

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jul 22, 2016

Yeah, it were broken on master, but already fixed.

@rafaelfranca rafaelfranca merged commit 0020f1a into rails:master Jul 22, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codeclimate Code Climate has skipped analysis of this commit.
Details
@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jul 22, 2016

Thank you for the pull request!

@jmccartie

This comment has been minimized.

Contributor

jmccartie commented Jul 23, 2016

@rafaelfranca Thanks for merging and for re-considering!

@matthewd

This comment has been minimized.

Member

matthewd commented Jul 23, 2016

"exclude?" is a nicer phrasing, for some uses, vs "not include?"; "not_in?" does not have the same merit over "not in?".

@jmccartie

This comment has been minimized.

Contributor

jmccartie commented Jul 23, 2016

@matthewd I don't follow. "not in" in the verbal (and logical opposite of) "in". Are you suggesting another phrase instead?

@matthewd

This comment has been minimized.

Member

matthewd commented Jul 23, 2016

@jmccartie my point is that ruby already has a perfectly satisfactory way of writing "not". The natural language opposite of "include" is "exclude", whereas, as you say, the natural language opposite of "in" is "not in" -- we don't need a new method for that, because that's exactly how !in? reads.

I don't see any argument for this that wouldn't equally apply to a similar opposing method for every other ? method.

@jmccartie

This comment has been minimized.

Contributor

jmccartie commented Jul 23, 2016

@matthewd Yup, I completely get your point. And I certainly wouldn't propose we create opposites for every ? method.

However, the original purpose of in? is to place the subject first in the query. For example, it's the English equivalent of changing "Home, are you in it?" to "Are you at home?". It's not a need, but it certainly helps with cognitive load when reading and writing code. I would liken it to unless -- another great way to reduce the overhead of if !something. Do we already have something for negation? Yes. Can we make a simple change to make developer's lives a little nicer, you bet.

And I agree with Rafael -- I'd love to see these methods (including Rails' in? and exclude?) added to Ruby core to go along with other language helpers that make Ruby a delight to write.

Cheers,
Jon

@matthewd

This comment has been minimized.

Member

matthewd commented Jul 23, 2016

Do we already have something for negation? Yes. Can we make a simple change to make developer's lives a little nicer, you bet.

I still haven't seen an argument that doesn't work just as well for is_not_a? / !is_a? or not_nil? / !nil?. in? exists to avoid a (sometimes) awkward subject/object inversion, which requires far more mental gymnastics than a floating 'not'... and if the latter is worthy of sugar, there's no reason we should stop at in?.

Perhaps you'd be better served by suggesting a syntax extension upstream, such that foo.!bar == !foo.bar. It's certainly true that "not foo is bar" is rarely the natural choice over "foo is not bar" -- but as long as that is the form allowed by the language syntax, and we have not chosen to globally "correct" it, a single method in that direction is going to create far more confusion than it prevents.

@dhh

This comment has been minimized.

Member

dhh commented Jul 29, 2016

I'd love to have a method for this, but not_in? doesn't clear my bar as nicer than !x.in?. We've tried several times in the past to come up with a great method name for this, but we've failed every time. not_in? was considered at every occasion.

You can read the long history of rejecting not_in? in #258, #12912, and #265.

@jmccartie

This comment has been minimized.

Contributor

jmccartie commented Jul 29, 2016

@dhh Very sad to see this get discussed, merged, then thrown away because of "history".

not_in? may not be linguistically 100% there, but it fills a need and I've gotten a lot of good feedback on it. Not sure the benefit of throwing this out completely. It's a very minor addition and doesn't change existing behavior. As mentioned earlier, it's easier to grok than !in? and completes the pairing between present?/blank?, include?/exclude? and in?/<nothing>.

For the record, this comment explains it perfectly. not_in? is self-documenting and works just fine in the English language.

@dhh

This comment has been minimized.

Member

dhh commented Jul 29, 2016

This isn't because of "history" but because of all the reasons examined in those prior PRs. From my perspective, there's no new information or arguments that we didn't already consider at length in 2011 and 2013, as per the referenced tickets.

Thankfully, Ruby and Rails make it oh-so-easy to include a method like not_in? on your own project. You don't need neither Matz' or my blessing to do so. If you like not_in?, then you can add it to your own dialect. That's all ActiveSupport is anyway. A dialect of Ruby intended for Rails development.

So please do have at it in your own project, and do return with a new PR if you get an epiphany on naming that was not previously considered. I like the functionality, but until there is a great name for it, I'd rather live without. (This principle of waiting for the right name, and rejecting features until we find them, has a long tradition in Ruby core development as well. One that I'd happy to continue).

@jmccartie

This comment has been minimized.

Contributor

jmccartie commented Jul 29, 2016

@dhh I humbly disagree. And I wish this could get discussed again.

But... I very much appreciate you taking the time to provide a lengthy response. Cheers. 🍻

@nickpoorman

This comment has been minimized.

nickpoorman commented Aug 1, 2016

I believe the term you're looking for is "exclusive".

@jmccartie

This comment has been minimized.

Contributor

jmccartie commented Aug 1, 2016

@nickpoorman For this behavior? No, I disagree. "not in" is typically used in English here. For example:

  • "His heart is not in it"
  • "The item is not in the list"
  • "This number is not in this array"
  • "This module is not in this class"

"exclusive" and "ex?" (as suggested in the other PR's) does not fit here, in my opinion.

@matthewd matthewd referenced this pull request Aug 10, 2016

Closed

create not_in? method #26112

@crazymykl

This comment has been minimized.

Contributor

crazymykl commented Sep 16, 2016

Object#out?

@maxp-edcast

This comment has been minimized.

maxp-edcast commented Feb 20, 2018

I don't want to open a new issue about it but I do want to give my 2 cents on it, even though it's a super old thread (sorry ..)

I have this code I want to refactor:

if params[:status].present? && User::PERMITTED_STATUSES.exclude?(params[:status])

I want to make it:

if params[:status]&.not_in? User::PERMITTED_STATUSES

In this example, since the safe navigation operator is used, !in? cannot simply be used instead of not_in?. So I would implore @dhh to reconsider this, yet again.

Granted, I could simply add it to our codebase, but I like to avoid too many core patches.

@dhh

This comment has been minimized.

Member

dhh commented Feb 20, 2018

@maxp-edcast We've gone over this a lot of times over the years. I respect this is something people are eager to have, but I haven't changed my opinion given that there's no new information available. You can write your example like:

if !params[:status]&.in?(User::PERMITTED_STATUSES)

Or

unless params[:status]&.in?(User::PERMITTED_STATUSES)
@maxp-edcast

This comment has been minimized.

maxp-edcast commented Feb 20, 2018

@dhh thanks for reading and responding. The examples you give are not the same as mine (if params[:status]&.not_in? User::PERMITTED_STATUSES) in the case that params[:status] is nil. With imaginary ruby syntax it would be if params[:status]&.!in(User::PERMITTED_STATUSES)

@dhh

This comment has been minimized.

Member

dhh commented Feb 20, 2018

Ah! All the more reason to write it in a less confusing way, then. Combine the try and the not is a pretty subtle read. I think the first example you had with an explicit present check is preferable.

@amerov

This comment has been minimized.

amerov commented Feb 27, 2018

It is may will to broke codebase which contain some_object.respond_to? :not_in?

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