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 Enumerable#without #19157

Merged
merged 1 commit into from Mar 2, 2015

Conversation

Projects
None yet
7 participants
@todd
Member

todd commented Mar 2, 2015

Re: #19082

I mostly took the code @dhh provided in the aforementioned issue and made a minor tweak after reviewing the relevant Ruby source and running some benchmarks - Array#reject is actually slower than Array#-, so I'm conditionally calling - on the enumerable if it's an Array. I can provide some benchmarks if anyone would like to see them. I'd also love for someone more well-versed in Ruby internals to take a look at this if they have the time.

@georgeclaghorn

View changes

Show outdated Hide outdated activesupport/lib/active_support/core_ext/enumerable.rb
# => ["David", "Rafael"]
def without(*elements)
if is_a? Array
self - elements

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Mar 2, 2015

Member

How about overriding this method separately in an Array core extension?

@georgeclaghorn

georgeclaghorn Mar 2, 2015

Member

How about overriding this method separately in an Array core extension?

This comment has been minimized.

@dhh

dhh Mar 2, 2015

Member

👍 to overwriting in an Array core extension. We do that with other methods like blank? etc.

@dhh

dhh Mar 2, 2015

Member

👍 to overwriting in an Array core extension. We do that with other methods like blank? etc.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 2, 2015

Member

Looking good besides for the type check 👏

Member

dhh commented Mar 2, 2015

Looking good besides for the type check 👏

@JuanitoFatas

View changes

Show outdated Hide outdated activesupport/lib/active_support/core_ext/enumerable.rb
#
# people = ["David", "Rafael", "Aaron", "Todd"]
# people.without "Aaron", "Todd"
# => ["David", "Rafael"]

This comment has been minimized.

@JuanitoFatas

JuanitoFatas Mar 2, 2015

Contributor

Add comment # before =>:

# => ["David", "Rafael"] ?

@JuanitoFatas

JuanitoFatas Mar 2, 2015

Contributor

Add comment # before =>:

# => ["David", "Rafael"] ?

This comment has been minimized.

@todd

todd Mar 2, 2015

Member

The other examples in this file use the same style.

@todd

todd Mar 2, 2015

Member

The other examples in this file use the same style.

This comment has been minimized.

@JuanitoFatas

JuanitoFatas Mar 2, 2015

Contributor

👌 Sorry.

@JuanitoFatas

JuanitoFatas Mar 2, 2015

Contributor

👌 Sorry.

This comment has been minimized.

@todd

todd Mar 2, 2015

Member

No worries! Happy to change this to be more consistent with preferred style in another PR 😉

@todd

todd Mar 2, 2015

Member

No worries! Happy to change this to be more consistent with preferred style in another PR 😉

@todd

View changes

Show outdated Hide outdated activesupport/lib/active_support/core_ext/array/grouping.rb
@@ -113,4 +113,8 @@ def split(value = nil)
results
end
end
def without(*elements) # :nodoc:

This comment has been minimized.

@todd

todd Mar 2, 2015

Member

Should we keep this as nodoc or add separate documentation for this from what's in enumerable.rb?

@todd

todd Mar 2, 2015

Member

Should we keep this as nodoc or add separate documentation for this from what's in enumerable.rb?

This comment has been minimized.

@dhh

dhh Mar 2, 2015

Member

I’d doc it and say what it does exactly as well as point to the enumerable version with a “this is an optimization of that”.

On Mar 1, 2015, at 18:18, Todd Bealmear notifications@github.com wrote:

In activesupport/lib/active_support/core_ext/array/grouping.rb:

@@ -113,4 +113,8 @@ def split(value = nil)
results
end
end
+

  • def without(*elements) # :nodoc:

Should we keep this as nodoc or add separate documentation for this from what's in enumerable.rb?


Reply to this email directly or view it on GitHub.

@dhh

dhh Mar 2, 2015

Member

I’d doc it and say what it does exactly as well as point to the enumerable version with a “this is an optimization of that”.

On Mar 1, 2015, at 18:18, Todd Bealmear notifications@github.com wrote:

In activesupport/lib/active_support/core_ext/array/grouping.rb:

@@ -113,4 +113,8 @@ def split(value = nil)
results
end
end
+

  • def without(*elements) # :nodoc:

Should we keep this as nodoc or add separate documentation for this from what's in enumerable.rb?


Reply to this email directly or view it on GitHub.

This comment has been minimized.

@todd

todd Mar 2, 2015

Member

Done.

@todd

todd Mar 2, 2015

Member

Done.

@dhh

View changes

Show outdated Hide outdated activesupport/lib/active_support/core_ext/array/grouping.rb
# people.without "Aaron", "Todd"
# => ["David", "Rafael"]
#
# Note: This is an optimization of `Enumerable#without`.

This comment has been minimized.

@dhh

dhh Mar 2, 2015

Member

Note: This is an optimization ofEnumerable#withoutthat uses Array#- instead of Enumerable#reject for performance reasons.

@dhh

dhh Mar 2, 2015

Member

Note: This is an optimization ofEnumerable#withoutthat uses Array#- instead of Enumerable#reject for performance reasons.

This comment has been minimized.

@todd

todd Mar 2, 2015

Member

Done - I changed Enumerable#reject to Array#reject since Array has its own implementation.

@todd

todd Mar 2, 2015

Member

Done - I changed Enumerable#reject to Array#reject since Array has its own implementation.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 2, 2015

Member

Great work on this 👍

Member

dhh commented Mar 2, 2015

Great work on this 👍

dhh added a commit that referenced this pull request Mar 2, 2015

@dhh dhh merged commit fc91616 into rails:master Mar 2, 2015

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 2, 2015

Member

Failing the build: https://travis-ci.org/rails/rails/jobs/52689785#L1077

We need to require what adds #in?.

Member

dhh commented Mar 2, 2015

Failing the build: https://travis-ci.org/rails/rails/jobs/52689785#L1077

We need to require what adds #in?.

@JuanitoFatas

This comment has been minimized.

Show comment
Hide comment
@JuanitoFatas

JuanitoFatas Mar 2, 2015

Contributor

@dhh Added in #19161.

Contributor

JuanitoFatas commented Mar 2, 2015

@dhh Added in #19161.

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Mar 2, 2015

Contributor

@JuanitoFatas the dependency require should be in code and not in test, otherwise the test doesn't test actual scenario with someone only including that specific file, which would break for them.

But given that this is low-level helper method, better to avoid another layer of abstraction and just check elements.include?(element).

Contributor

egilburg commented Mar 2, 2015

@JuanitoFatas the dependency require should be in code and not in test, otherwise the test doesn't test actual scenario with someone only including that specific file, which would break for them.

But given that this is low-level helper method, better to avoid another layer of abstraction and just check elements.include?(element).

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 2, 2015

Member

Wups. I should have caught that. What @egilburg said.

Member

dhh commented Mar 2, 2015

Wups. I should have caught that. What @egilburg said.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 2, 2015

Member

Even better 👍

On Mar 1, 2015, at 19:21, Eugene Gilburg notifications@github.com wrote:

@JuanitoFatas the dependency require should be in code and not in test, otherwise the test doesn't test actual scenario with someone only including that specific file.

But given that this is low-level helper method, better to avoid another layer of abstraction and just check elements.include?(element).


Reply to this email directly or view it on GitHub.

Member

dhh commented Mar 2, 2015

Even better 👍

On Mar 1, 2015, at 19:21, Eugene Gilburg notifications@github.com wrote:

@JuanitoFatas the dependency require should be in code and not in test, otherwise the test doesn't test actual scenario with someone only including that specific file.

But given that this is low-level helper method, better to avoid another layer of abstraction and just check elements.include?(element).


Reply to this email directly or view it on GitHub.

@JuanitoFatas

This comment has been minimized.

Show comment
Hide comment
@JuanitoFatas

JuanitoFatas Mar 2, 2015

Contributor

@egilburg @dhh Sorry let me fix that.

Contributor

JuanitoFatas commented Mar 2, 2015

@egilburg @dhh Sorry let me fix that.

@todd

This comment has been minimized.

Show comment
Hide comment
@todd

todd Mar 2, 2015

Member

Huh - didn't run across that in my own testing. Sorry about that. I like @egilburg's solution as well.

Member

todd commented Mar 2, 2015

Huh - didn't run across that in my own testing. Sorry about that. I like @egilburg's solution as well.

@todd todd deleted the todd:enumerable_without branch Mar 2, 2015

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Mar 2, 2015

Member

grouping.rb?

Member

matthewd commented Mar 2, 2015

grouping.rb?

@todd

This comment has been minimized.

Show comment
Hide comment
@todd

todd Mar 2, 2015

Member

Seemed like the most appropriate place to put the new method given the presence of #split in the same file. Is there a better location you can think of?

Member

todd commented Mar 2, 2015

Seemed like the most appropriate place to put the new method given the presence of #split in the same file. Is there a better location you can think of?

dhh added a commit that referenced this pull request Mar 2, 2015

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 2, 2015

Member

@matthewd Agree, moved it to Access. No worries, @todd it wasn't immediately clear where it should go. But I think it belongs between in access.

Member

dhh commented Mar 2, 2015

@matthewd Agree, moved it to Access. No worries, @todd it wasn't immediately clear where it should go. But I think it belongs between in access.

@todd

This comment has been minimized.

Show comment
Hide comment
@todd

todd Mar 2, 2015

Member

👍

Member

todd commented Mar 2, 2015

👍

tenderlove added a commit that referenced this pull request Mar 5, 2015

Merge branch 'master' into url_context
* master: (63 commits)
  Revert "delete unused method"
  Revert "mutate the transaction object to reflect state"
  be optimistic about missing route keys
  use arg size for parallel iteration
  ask the routes objects for its Rack env key
  delete unused method
  mutate the transaction object to reflect state
  ask the txn for it's state, not a state object
  change if! to unless
  Remove unneeded comment. [ci skip]
  Move Array#without from Grouping to Access concern and add dedicated test (relates to #19157)
  tests, favor `drop_table` and `:if_exists` over raw SQL.
  Skip the failing tests on Rubinius for now
  Remove not needed .tap
  Wrap inline rescue with or-equal calls
  [ci skip] Fix a typo for PostgreSQL text limit, GB instead of Gb.
  Avoid Ruby versions check on Rubinius
  Make private methods private
  Remove !has_transactional_callbacks? check
  Test against the mail gem's edge
  ...

Conflicts:
	actionpack/lib/action_dispatch/http/url.rb
	actionpack/lib/action_dispatch/routing/route_set.rb
@aripollak

This comment has been minimized.

Show comment
Hide comment
@aripollak

aripollak Mar 9, 2015

Contributor

Has there been any talk about moving this to except to mirror Hash#except, since the behavior should already be the same for Hashes?

Contributor

aripollak commented Mar 9, 2015

Has there been any talk about moving this to except to mirror Hash#except, since the behavior should already be the same for Hashes?

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 9, 2015

Member

That's a good thought, but I don't think it reads as naturally: @people.without(Current.creator) reads better to me than @people.except(Current.creator). If anything, maybe it's a pointer that Hash#except should have a #without alias when that's the more natural flow.

Member

dhh commented Mar 9, 2015

That's a good thought, but I don't think it reads as naturally: @people.without(Current.creator) reads better to me than @people.except(Current.creator). If anything, maybe it's a pointer that Hash#except should have a #without alias when that's the more natural flow.

@aripollak

This comment has been minimized.

Show comment
Hide comment
@aripollak

aripollak Mar 9, 2015

Contributor

I think they both read fine 😁

Contributor

aripollak commented Mar 9, 2015

I think they both read fine 😁

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Mar 9, 2015

Member

We’re not going for “fine” here ;). It’s a comparative study and to me there’s no contest: The code reads much clearer with #without.

On Mar 9, 2015, at 15:13, Ari Pollak notifications@github.com wrote:

I think they both read fine


Reply to this email directly or view it on GitHub.

Member

dhh commented Mar 9, 2015

We’re not going for “fine” here ;). It’s a comparative study and to me there’s no contest: The code reads much clearer with #without.

On Mar 9, 2015, at 15:13, Ari Pollak notifications@github.com wrote:

I think they both read fine


Reply to this email directly or view it on GitHub.

brainopia added a commit to brainopia/rails that referenced this pull request Mar 25, 2015

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