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 Iterator#slice_after #7146

Merged
merged 1 commit into from
Dec 5, 2018
Merged

Conversation

asterite
Copy link
Member

@asterite asterite commented Dec 4, 2018

Part of #7142

@asterite
Copy link
Member Author

asterite commented Dec 5, 2018

Note that this PR is complete by itself. I will add the other methods slice_before, slice_when after this one is merged. I might reuse some code between them, but I think adding one PR for each method will be easier to review.

@bew
Copy link
Contributor

bew commented Dec 5, 2018

I don't understand why it's called slice_after based on what it does.. I seems better to me to call it slice_until, as it'll add things to the slice until the block is true.

@asterite
Copy link
Member Author

asterite commented Dec 5, 2018

@bew After it finds the value it "slices" the enumerable. Or put another way, it slices the enumerable after each time returns a truthy value.

@bew
Copy link
Contributor

bew commented Dec 5, 2018

Ooh ok I get it, is it possible to add an explanation like this in the doc? I think it'd help memorize the naming logic (and probably help with the other iterator you're planning to add)

@bcardiff
Copy link
Member

bcardiff commented Dec 5, 2018

An spec for why the reuse is not Bool | Array(B) might be worth it ;-)

@asterite
Copy link
Member Author

asterite commented Dec 5, 2018

Yes, I thought about putting a restriction to reuse, but none of the other methods have that restriction. I'll add it just for this one and in a separate PR I'll add the rest of the restrictions.

@asterite asterite force-pushed the feature/slice_after branch 2 times, most recently from e9c4d8b to 4d1d52d Compare December 5, 2018 11:43
@asterite
Copy link
Member Author

asterite commented Dec 5, 2018

Updated:

  • put a restriction on reuse
  • document reuse

@asterite
Copy link
Member Author

asterite commented Dec 5, 2018

Also added a small explanation to why it's called slice_after.

src/iterator.cr Outdated
include Iterator(Array(T))

def initialize(@iterator : I, @block : T -> B, reuse)
@values = [] of T
Copy link
Contributor

@bew bew Dec 5, 2018

Choose a reason for hiding this comment

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

if reuse is an Array, it'll still create an empty array here for nothing :(
I think you can remove this line

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Fixed! That's the code I originally had before adding all the reuse logic. Thank you!

@asterite asterite force-pushed the feature/slice_after branch from 4d1d52d to 58ff560 Compare December 5, 2018 12:58
Copy link
Contributor

@bew bew left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@bcardiff
Copy link
Member

bcardiff commented Dec 5, 2018

@asterite I liked the no restriction actually. I thought it was intentional to allow more flexibility: For example use an Array(Int32 | String) as a reuse in a slice_after over an Array(Int32). It is assignable from Int32 and that is enough.

@bcardiff
Copy link
Member

bcardiff commented Dec 5, 2018

For the record: removing the restriction is not possible (or not worth the effort).

@asterite asterite merged commit 3021206 into crystal-lang:master Dec 5, 2018
@asterite asterite added this to the 0.27.1 milestone Dec 5, 2018
@asterite asterite deleted the feature/slice_after branch December 5, 2018 17:48

if reuse
if reuse.is_a?(Array)
@values = reuse
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't @clear_on_next = true here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm... you are supposed to pass an empty array as a reuse value. It's not documented what happens if you pass an array that has some values already, but in this case they will be part of the first chunk. Maybe that's good. I wouldn't worry about this now.

Copy link
Contributor

@RX14 RX14 Dec 7, 2018

Choose a reason for hiding this comment

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

The obvious usecase for passing an array as reuse is when you've just used the array from another iterator, meaning the array might not be empty. I don't think it's good to pass the burden of calling clear onto the caller, since it's easy to forget. Also, clear on an already-empty array is very cheap, so why not?

Copy link
Member Author

Choose a reason for hiding this comment

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

What if you want to get chunks but want the first chunk to have some elements beforehand? If the iterator calls clear there's no way you can do that.

I think we are just inventing use cases. Whenever I used reuse I passed a boolean. We can maybe remove the ability to pass an array altogether.

Copy link
Contributor

@RX14 RX14 Dec 7, 2018

Choose a reason for hiding this comment

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

If you want the first chunk to have extra elements:

  1. that's pretty inflexible, since you can only do it with the first chunk, not the last
  2. using a named arg called reuse: is not the most clear way to achieve this.

For me, reuse is for avoiding memory allocations only. That's it's only usecase. Always calling clear makes that usecase easier with very small extra expense.

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.

4 participants