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

Add Enumerable#each_step and Iterable#each_step #13610

Merged
merged 8 commits into from
Oct 18, 2023

Conversation

baseballlover723
Copy link
Contributor

Fixes #13583

spec/std/enumerable_spec.cr Outdated Show resolved Hide resolved
src/enumerable.cr Outdated Show resolved Hide resolved
src/enumerable.cr Outdated Show resolved Hide resolved
src/iterable.cr Outdated Show resolved Hide resolved
src/iterable.cr Outdated Show resolved Hide resolved
@straight-shoota straight-shoota changed the title implemented Enumerable#each_step and Iterable#each_step Add Enumerable#each_step and Iterable#each_step Jul 5, 2023
@baseballlover723
Copy link
Contributor Author

is there anything else that needs updates at this point? It doesn't seem like there is anything outstanding, but it isn't approved yet.

src/enumerable.cr Outdated Show resolved Hide resolved
src/iterable.cr Outdated Show resolved Hide resolved
spec/std/enumerable_spec.cr Outdated Show resolved Hide resolved
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

❤️

Comment on lines 405 to 421
it "doesn't accept a negative step" do
expect_raises(ArgumentError) do
%w[a b c d e f].each_step(-2)
end
end

it "doesn't accept a step of 0" do
expect_raises(ArgumentError) do
%w[a b c d e f].each_step(0)
end
end

it "doesn't accept a negative offset" do
expect_raises(ArgumentError) do
%w[a b c d e f].each_step(2, offset: -2)
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

These specs only cover exceptions raised by Iterator#skip and Iterator#step, the Enumerable#each_step forms are not tested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, I had it in my mind that the iterator validation would carry over. I've added the validation to Enumerable#each_step.

describe "each_step" do
it_iterates "yields every 2nd element", %w[a c e], %w[a b c d e f].each_step(2)
it_iterates "accepts an optional offset parameter", %w[b d f], %w[a b c d e f].each_step(2, offset: 1)
it_iterates "accepts an optional offset parameter of 0", %w[a c e], %w[a b c d e f].each_step(2, offset: 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like extra spec(s) where n > 0 && (n <= offset < size), which would skip some elements at the beginning

src/enumerable.cr Outdated Show resolved Hide resolved
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
@baseballlover723
Copy link
Contributor Author

@HertzDevil can you take another look at this?

@baseballlover723
Copy link
Contributor Author

@HertzDevil I hate to have to ping you again for this, but I believe this is ready to be reviewed / merged now.

@baseballlover723
Copy link
Contributor Author

@HertzDevil can you review this again? I think it's ready to be merged and I'd like to get it in for 1.10.0.

@straight-shoota straight-shoota added this to the 1.11.0 milestone Oct 16, 2023
@straight-shoota straight-shoota merged commit 9e625d5 into crystal-lang:master Oct 18, 2023
51 of 52 checks passed
@baseballlover723 baseballlover723 deleted the pr/each_step branch October 18, 2023 18:07
Blacksmoke16 pushed a commit to Blacksmoke16/crystal that referenced this pull request Dec 11, 2023
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enumerable and Iterable forms of Iterator#step
5 participants