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-5786: Fix some compatibility issues with Enumerable API #5831

Merged
merged 3 commits into from
Jun 17, 2024

Conversation

cperezabo
Copy link
Contributor

Hi, in accordance with Ruby's enumerable API, I've added support for initial value when a block is given to Enumerable#sum.

Tell me if the provided changes are ok and I could replicate the same changes to min and max methods.

Cheers!

@comandeo-mongo comandeo-mongo changed the title Fix some compatibility issues with Enumerable API MONGOID-5786: Fix some compatibility issues with Enumerable API Jun 17, 2024
Copy link
Contributor

@jamis jamis left a comment

Choose a reason for hiding this comment

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

Thank you! Good catch, and thanks too for the tests. 👍

@cperezabo
Copy link
Contributor Author

cperezabo commented Jun 17, 2024

Hi @jamis, how correct is to specify the Numeric type to the parameter? Because in fact, you can pass any kind of object that implements :+ as initial value, not only Numeric. Should it simply be Object?

@jamis
Copy link
Contributor

jamis commented Jun 17, 2024

@cperezabo -- good question! You're correct that the nature of Ruby means that anything that quacks like a number (or which responds to :+, as you said) could work as a parameter here. The point of the documentation here is less to strictly specify the types, than to imply use cases. Here, we say "Numeric", because of what it suggests about the interface, and less because we strictly require numbers.

If we specified Object, instead, it would communicate very little about the expected contract.

In a perfect world, we would document argument's interface, which is possible, but gets verbose quickly. For now, specifying [ Symbol | Numeric ] is succinct, and suggests the allowed interfaces.

@jamis jamis merged commit 08547c3 into mongodb:master Jun 17, 2024
29 of 32 checks passed
@cperezabo
Copy link
Contributor Author

One more thing, this bug is also present in min and max methods. Shouldn't have be done in this PR?
Now that this one is merged, are you going to implement the changes in those methods or do you need me to do that?

@jamis
Copy link
Contributor

jamis commented Jun 17, 2024

Fixing them ourselves is not a high priority; unless you intend to address those methods sooner, we'll just add a ticket and put it in our backlog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants