Add Enumerable#without #19157
Add Enumerable#without #19157
Conversation
| # => ["David", "Rafael"] | ||
| def without(*elements) | ||
| if is_a? Array | ||
| self - elements |
There was a problem hiding this comment.
How about overriding this method separately in an Array core extension?
There was a problem hiding this comment.
👍 to overwriting in an Array core extension. We do that with other methods like blank? etc.
|
Looking good besides for the type check 👏 |
| # | ||
| # people = ["David", "Rafael", "Aaron", "Todd"] | ||
| # people.without "Aaron", "Todd" | ||
| # => ["David", "Rafael"] |
There was a problem hiding this comment.
Add comment # before =>:
# => ["David", "Rafael"] ?
There was a problem hiding this comment.
The other examples in this file use the same style.
There was a problem hiding this comment.
No worries! Happy to change this to be more consistent with preferred style in another PR 😉
|
Great work on this 👍 |
|
Failing the build: https://travis-ci.org/rails/rails/jobs/52689785#L1077 We need to require what adds #in?. |
|
@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 |
|
Wups. I should have caught that. What @egilburg said. |
|
Even better 👍
|
|
Huh - didn't run across that in my own testing. Sorry about that. I like @egilburg's solution as well. |
|
|
|
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? |
|
👍 |
* 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
|
Has there been any talk about moving this to |
|
That's a good thought, but I don't think it reads as naturally: |
|
I think they both read fine 😁 |
|
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.
|
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#rejectis actually slower thanArray#-, so I'm conditionally calling-on the enumerable if it's anArray. 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.