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 Enumerable#sole #40914

Merged
merged 3 commits into from
Apr 19, 2021
Merged

Add Enumerable#sole #40914

merged 3 commits into from
Apr 19, 2021

Conversation

kivikakk
Copy link
Contributor

This adds the Enumerable sole definition moreorless exactly per @matthewd's comment at #26206 (comment). I've left the block default out for simplicity's sake, but we could add it if we feel the utility's there. If we do, it needs to be added to #40768 as well.

This is an add-on to #40768 "Add ActiveRecord::FinderMethods#sole and #find_sole_by"; this will slide in underneath the FinderMethod's #sole. It seems a bit ugly that the exceptions raised by the method will change from Enumerable::SoleItemExpectedError to ActiveRecord::SoleRecordExceeded, but it also seems most correct/least incorrect.

/cc @dhh @matthewd

# { a: 1, b: 2 }.sole # => Enumerable::SoleItemExpectedError: multiple items found
def sole
it = nil
found = false
Copy link
Member

Choose a reason for hiding this comment

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

Can't we use the same technique of first(2)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made this change, but realised it doesn't let us distinguish these cases:

irb(main):001:0> found, undesired = [:a].first(2)
=> [:a]
irb(main):002:0> found
=> :a
irb(main):003:0> undesired
=> nil
irb(main):004:0> found, undesired = [:a, nil].first(2)
=> [:a, nil]
irb(main):005:0> found
=> :a
irb(main):006:0> undesired
=> nil
irb(main):007:0> 

As it stands, sole will now not raise if an Enumerable yields two items, the second of which is nil.

The original change per #26206 (comment) does distinguish this case, so I think I prefer it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted the change of algorithm in 0c58f6f817.

Copy link
Member

Choose a reason for hiding this comment

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

That strikes me as more confusing, to be honest. That an extra element that happens to be nil isn't considered an element. I'd rather we treat nil as an element in this evaluation. So I think we should revert to the first(2) approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd rather we treat nil as an element in this evaluation. So I think we should revert to the first(2) approach.

I'm a bit confused by this comment.

I've reverted to the first(2) approach as requested, but — per the example irb session above — a second item which happens to be nil is not distinguished from no second item at all, as x, y = first(2) will put a nil into the y in either case. As a result, [:a, nil].sole == :a.

The alternative implementation (kivikakk@a7c90384c77) causes a SoleItemExpectedError in this case. I think that's more correct and less likely to cause unexpected errors, but happy to defer to you here.

Copy link
Member

Choose a reason for hiding this comment

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

Let's make a point to do show that rebelling against Rubocop is possible ✊. So please do add that per-line exemption, and let's put the return back in there. It's this level of finesse and subtlety that brings charm and joy to writing Ruby 😄❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome. Done!

Copy link
Member

Choose a reason for hiding this comment

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

Can we use size instead of count?
Unlike size, count on a relation will do a COUNT query even if a relation is loaded.

Copy link
Member

Choose a reason for hiding this comment

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

AR already has its own implementation of #sole. This is for other Enumerables.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I didn't know that (#40768), thank you for letting me know!

Base automatically changed from master to main January 14, 2021 17:02
@rails-bot
Copy link

rails-bot bot commented Apr 14, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Apr 14, 2021
@kivikakk
Copy link
Contributor Author

Rebased on latest main.

@rails-bot rails-bot bot removed the stale label Apr 15, 2021
@dhh dhh merged commit 2e14c53 into rails:main Apr 19, 2021
@dhh
Copy link
Member

dhh commented Apr 19, 2021

👏

@kivikakk kivikakk deleted the sole-enumerable branch April 20, 2021 00:18
@kivikakk
Copy link
Contributor Author

@dhh Thanks for your time and care in reviewing, I really appreciate it! ❤️

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.

None yet

4 participants