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

MONGOID-4914 Unmask Enumerable#find #4802

Closed
wants to merge 4 commits into from

Conversation

rgould
Copy link
Contributor

@rgould rgould commented Jul 4, 2020

# The signature of Enumerable#find:
#
# find(ifnone = nil) { |obj| block } → obj or nil
# find(ifnone = nil) → an_enumerator
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not need to duplicate standard library documentation in our docstrings. Linking to stdlib docs I would say is sufficient and this way our documentation cannot get out of date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call - fixed at b58cf1d

# criteria.find { |item| item.name == "Depeche Mode" }
#
# @example Find given a block and a Proc, invokes Enumerable#find
# criteria.find(-> { "Default Band" }) { |item| item.name == "Milwaukee Mode" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there actual usage of this pattern in the wild, considering one can just as easily say

     #   criteria.find { |item| item.name == "Milwaukee Mode" } || 'Default Band'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I've never seen it. I was just trying to cover all the cases for Enumerable#find :)

# @return Same return value as either Findable#find or Enumerable#find, depending
# on which one was invoked.
#
# @since 7.1.1
Copy link
Contributor

Choose a reason for hiding this comment

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

We no longer use since tags, and this one isn't accurate since the change would go into the next major release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed at 397f4c2

#
# @since 7.1.1
def find(*args, &block)
if block_given? || args.first&.is_a?(Proc)
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose changing this to if block_given? only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine by me :) Fixed at c08d43f

@p-mongo
Copy link
Contributor

p-mongo commented Jul 20, 2020

Hi @rgould , thank you for making these changes.

I looked into this PR in #4817 and it seems the changes so far have to do with association finds and the top-level model finds seem to also be affected by the same issue:

irb(main):005:0> Band.find { |x| p x }
=> nil

Would you like to investigate the top-level find behavior? It looks like you added unit tests for Criteria and it seems we also need integration tests for finding on an association as well as finding on top level.

@rgould
Copy link
Contributor Author

rgould commented Jul 22, 2020

Ah! Sure! I don't know how but I convinced myself it wasn't necessary. I'll try and take a look at it this weekend :) Thanks!

@johnnyshields
Copy link
Contributor

Should close in favor of #4817

@p-mongo
Copy link
Contributor

p-mongo commented Aug 9, 2020

@rgould Are you able to make the changes mentioned? If not we can also do them.

@rgould
Copy link
Contributor Author

rgould commented Aug 12, 2020

@p-mongo Sorry! Lots of surprise bureaucracy is getting in my way. I'll try and get it done this weekend, but if I don't, feel free to take it on if you have the capacity

@rgould
Copy link
Contributor Author

rgould commented Aug 16, 2020

@p-mongo I started to look into this again today, and I'm not exactly sure where I should make this change. Also note that ActiveRecord doesn't document this feature on their end, though they do incidentally support it.

Do we want to make Document or maybe Composable also include Enumerable? If it's Composable, then the solution is consistent with how it was done for Criteria

PS. I don't have a lot of time in the next couple weeks, so if you want to pick this up and finish it I'm totally okay with that. If not I will come back to it when I find the time :)

@p-mongo
Copy link
Contributor

p-mongo commented Oct 6, 2020

I filed https://jira.mongodb.org/browse/MONGOID-5003 for the top-level find change, and this PR will be merged as #4817.

@p-mongo p-mongo closed this Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants