Skip to content

Remove implementation detail specs for Enumerator#1361

Open
Earlopain wants to merge 1 commit into
ruby:masterfrom
Earlopain:enumerator-internals
Open

Remove implementation detail specs for Enumerator#1361
Earlopain wants to merge 1 commit into
ruby:masterfrom
Earlopain:enumerator-internals

Conversation

@Earlopain
Copy link
Copy Markdown
Contributor

@Earlopain Earlopain commented May 19, 2026

Enumerator::Yielder, Enumerator::Generator and Enumerator::Producer are not supposed to be exposed to the user.
Instead they will interact with them trough other means, like for example Enumerator.new { |yielder| }.

I also have this PR to nodoc them ruby/ruby#17036 (they are already basically just present by name only)

@eregon
Copy link
Copy Markdown
Member

eregon commented May 19, 2026

I agree they shouldn't be documented (and merged you ruby/ruby PR) but they are still public API I would say, and based on that I think it's fair enough to test them in ruby/spec.
The are exposed to the user, e.g.:

$ ruby -e 'Enumerator.new { |y| p y }.take(1)'
#<Enumerator::Yielder:0x00007fac648b5a48>

So I wouldn't call the class names implementation details since they are public constants.

Is there something specific these specs test you think they shouldn't?

From a quick look core/enumerator/generator/initialize_spec.rb and core/enumerator/yielder/initialize_spec.rb seem rather useless except the first one checks initialize can't be called on a frozen instance which is something.
The rest looks not too unreasonable to me from a quick look.

@eregon
Copy link
Copy Markdown
Member

eregon commented May 19, 2026

In general the philosophy of core specs is "spec every public method of every public class/module" (and also some private methods like initialize). It's actually what mkspec does.

@eregon
Copy link
Copy Markdown
Member

eregon commented May 19, 2026

I do like the new specs though, so maybe we'd only remove the useless initialize specs but keep the rest?

@Earlopain
Copy link
Copy Markdown
Contributor Author

I haven't (intentionally) removed any specs:

it "returns self" do
  y = Enumerator::Yielder.new {|x| x + 1}
  (y << 1).should.equal?(y)
end

# into

it 'can be chained' do
  enum = Enumerator.new do |y|
    y << 1 << 2
  end
  enum.to_a.should == [1, 2]
end

Which will be how people in theory actually use it. If MRI for some reason decides to return Enumerator::DifferentYielder, then nobody will really notice, except perhaps in inspect output because the name is different. Theoretically a different ruby implementation might also not implement it in this way (although I don't think that is the case, seems there is something to this approach).

I see this more like ducktyping. The concrete class doesn't matter, as long as it behaves correctly, so I rewrote the specs to how it's actually used. No one is directly instantiating Enumerator::Yielder and calling <</yield on it. But if that is how ruby-spec does it, I won't argue. Just let me know if I should still revert removing the existing specs.

`Enumerator::Yielder`, `Enumerator::Generator` and `Enumerator::Producer` are not
supposed to be exposed to the user.
Instead they will interact with them trough other means,
like for example `Enumerator.new { |yielder| }`.
@Earlopain Earlopain force-pushed the enumerator-internals branch from 8a6da41 to 056dcf3 Compare May 27, 2026 11:41
@Earlopain
Copy link
Copy Markdown
Contributor Author

I moved the new specs to truffleruby/truffleruby#4271 since that is a less controversial change

Earlopain added a commit to Earlopain/truffleruby that referenced this pull request May 27, 2026
@eregon
Copy link
Copy Markdown
Member

eregon commented May 27, 2026

Taking another look at this.
I see in truffleruby/truffleruby#4271 we have specs for Enumerator::Yielder #<< and #yield, but this PR removes many more files.
Are these all redundant with existing specs?
From the current diff and previous diff it's not clear to me if they are.

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