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

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| }.

Some specs I ported, others I removed, and some were already present.

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

`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| }`.

Some specs I ported, others I removed, and some were already present.
@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?

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