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

NodeSet#each returns zero instead of self #1822

Closed
olehif opened this issue Nov 30, 2018 · 1 comment
Closed

NodeSet#each returns zero instead of self #1822

olehif opened this issue Nov 30, 2018 · 1 comment
Milestone

Comments

@olehif
Copy link
Contributor

olehif commented Nov 30, 2018

According to ruby convention, #each method should return self:
[1, 2].each {} # => [1, 2]

While NodeSet#each always return zero instead:
nodes.each {} # => 0

As everyone in the ruby community expects #each to always return self (I myself had a hard time debugging seemingly perfect line of code) I think it would make sense to change this behavior even though it might potentially be a breaking change.

@flavorjones
Copy link
Member

Hey! Thanks for submitting this. I agree it's unexpected behavior to not act like an Enumerable or Array class, which is what NodeSet is intended to behave like.

That said, I'll first (and this is intended to be humorous!) point out that this is not an obvious assumption for "everyone in the ruby community": see #1678 where some people have opined that at least one aspect of Enumerable's behavior is non-intuitive.

Second, I do want to point out that this is slightly more complicated than you've presented. The Array#each method returns an Enumerator object when there's no block presented. This PR doesn't address that behavior, and that's OK, I just want to point out that there's a complex set of behaviors that despite our best intentions may never get fully implemented in NodeSet.

Further evidence of this complexity is in #1677 where I honestly failed to completely understand the issue(s) being submitted.

In any case, this is an awesome, quick fix for what I consider to be a bug. Thank you! Will merge and this and it should be in the next release! Will give you a mention in the CHANGELOG.

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

No branches or pull requests

2 participants