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

Extend specs of Set#divide #1049

Merged
merged 2 commits into from
Aug 9, 2023
Merged

Extend specs of Set#divide #1049

merged 2 commits into from
Aug 9, 2023

Conversation

herwinw
Copy link
Member

@herwinw herwinw commented Aug 5, 2023

This contains two commits:

  1. Add test for calling Set#divide without a block. There was a commented piece of code that expected an exception, but since Ruby 2.4 the documentation states it should return an Enumerator
  2. Add test for calling the block with an undefined arity. The docs describe the behaviour for blocks with arity 1 and 2, but not what happens when call the method with a block with a different arity. This is technically undefined behaviour, so I'm not sure whether this spec should be added.

The description in the docs state "Returns an enumerator if no block is
given", added in Ruby 2.4.
it "only uses the first element if the arity is not 2" do
set = Set["one", "two", "three", "four", "five"].divide { |x, _, _| x.length }
set.map { |x| x.to_a.sort }.sort.should == [["five", "four"], ["one", "two"], ["three"]]
end
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: I would move out this case into a separate describe/`context' (with a title like "when passed a block with an arity > 2") for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Also I like the check in the case above:

ret.each(&:even?).should == Set[Set[1, 3], Set[2, 4]]

using Set instead of sorting looks more readable and shorted

Copy link
Member

Choose a reason for hiding this comment

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

suggestion: TBH I would find a case title a bit laconic and not comprehensive (as well as in a similar case below for arity = 2). It will be great if the titles reflect documentation and describe exactly what happens and how a set elements are compared/divided:

If the arity of the block is 2, elements o1 and o2 are in common if block.call(o1, o2) is true. Otherwise, elements o1 and o2 are in common if block.call(o1) == block.call(o2).

Copy link
Member Author

Choose a reason for hiding this comment

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

suggestion: I would move out this case into a separate describe/`context' (with a title like "when passed a block with an arity > 2") for consistency.

I extracted this one into its own block, and added a block with a splat argument as well (which also should use the first element, since this results in an arity of -1, this might be a nice extra test case for Ruby implementations)

I think the other two entries are nice additions, but slightly out of scope for this case.

@andrykonchin andrykonchin merged commit 958e32b into ruby:master Aug 9, 2023
10 checks passed
@andrykonchin
Copy link
Member

Thank you for the specs!

@herwinw herwinw deleted the set_divide branch August 9, 2023 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants