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

RFC add foreach to iterators #582

Closed
wants to merge 2 commits into from
Closed

RFC add foreach to iterators #582

wants to merge 2 commits into from

Conversation

larroy
Copy link

@larroy larroy commented Jan 13, 2015

No description provided.

@tshepang
Copy link
Member

nicer view


# Detailed design

The design is simple, just syntacting suggar around a loop that consumes the iterator as seen in
Copy link

Choose a reason for hiding this comment

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

I believe you mean "syntactic sugar" here.

@sinistersnare
Copy link

i think foreach does not work well with rust naming conventions, so I would say either for_each, or just each. I like each.

@blaenk
Copy link
Contributor

blaenk commented Jan 14, 2015

I too prefer each, as in Ruby.

@jsanders
Copy link

I think this would be really nice. 👍 to each from me too.

Side note: I had to do some clicking around to figure out what is being proposed here. Could you put the main idea in the RFC itself?

@mneumann
Copy link

+1 each

@SimonSapin
Copy link
Contributor

@SimonSapin
Copy link
Contributor

However, since for loops are expressions, it’s not clear to me how a method doing the same is better.

@jsanders
Copy link

@SimonSapin: I had the same thought at first, but this comment on reddit convinced me that it can be quite nice to end a chain of iterator methods with an application of a closure instead of splitting it up into a separate iterator chain plus a for loop. But maybe it really just belongs in itertools, where it already is.

@SimonSapin
Copy link
Contributor

While this is interesting, I think the way to go at this point is to demonstrate wide usage in a Cargo crate like itertools. The bar should be high to move thing into std.

@blaenk
Copy link
Contributor

blaenk commented Jan 15, 2015

I don't think that would be the best metric for measuring desire in this feature. I personally think this would be very convenient, but not to the point where I would necessarily bring in a separate dependency just for that.

I agree that the bar should be high, but this is a very simple addition, and one that is a core part of many other languages that have similar iteration/chaining/streams APIs (such as Scala, C#, Java 8, and Ruby to name a few).

I think the RFC would benefit from including a code example motivating the addition of this method. While I think for loops are great, I do think it's nice to not have to break the chain of iteration methods just to force the side-effecting changes to go through.

let mut somechain = xs.iter().take(5).filter(|x| x.is_thing()).map(|x| x + 1);

for thing in somechain {
  // do side effecting operation
  println!("{:?}", x);
}

It would be nicer to be able to just do this for certain short cases:

xs.iter()
  .take(5)
  .filter(|x| x.is_thing())
  .map(|x| x + 1)
  .each(|x| println!("{:?}", x));

I'm not adamant about including this, but it did always strike me as odd that this method was missing from iterators.

@SimonSapin
Copy link
Contributor

I personally think this would be very convenient, but not to the point where I would necessarily bring in a separate dependency just for that.

I used to think that way. And then I learned to love Cargo. Publishing and using very small crates like matches! is fine! If you’re already using Cargo, what is really the cost of an additional dependency?

@SimonSapin
Copy link
Contributor

That said some things still should go into std eventually. But in the mean time, real world usage experience can bring feedback of changes you might want to do before setting thing is backward-compatibility stone.

@blaenk
Copy link
Contributor

blaenk commented Jan 16, 2015

@SimonSapin Oh that's not what I meant. I don't mind bringing in another dependency if the thing I want to use is necessary and useful. What I meant was that in my case, and I bet other people, if this method were available to me out of the box I would readily use it, but otherwise I don't care enough about it to warrant bringing in a dependency simply for it.

Take your matches! macro for example, I loved it and was disappointed when it wasn't accepted in the std. There are times when I would have liked to have used it, but to date I haven't bothered with actually bringing in that dependency, simply because I don't care enough, and so far it hasn't seemed overly inconvenient to just write it out explicitly. But, again, if it were there then I would use it.

With that in mind, it's also why I wouldn't mind too much if this weren't accepted, but it does feel like it's conspicuously missing from the list of iterator methods to others who are familiar with similar iterator APIs.

And I still agree with you that we should be careful/cautious about the things that are proposed to be added to the std; I wholeheartedly agree. I think it's a good idea to, like you say, first get feedback about such proposals, but I think that's already what this RFC is for; it's not really an earth-shattering proposal with deep and complex ramifications, AFAIK.

We do have to draw the line somewhere. After all, if we accept this, then what's not to stop us from adding a bunch of other simple proposals until the std is undirect and bloated, right? Sure, but my argument is that this method is a very common component of similar APIs, I think of it as an iteration 'primitive' (such as map, fold, etc.) which has precedent in other established languages.

@larroy: Please include a code example showing specifically how this may be useful. Feel free to use mine or a better one you could come up with. It's nice to have self-contained RFCs so people could easily get a full picture of what you intend, even if the use of your proposed method seems very obvious to you.

@thestinger
Copy link

This already existed (it was called advance) and after a lot of deliberation if was removed because it's almost entirely redundant with for loops. Rust has tried to be a clean language with one way to do things. It has to contend with the need to expose lots of flexibility for performance reasons, but I think it has done very well so far - and part of that is because it has rejected redundant methods like this.

Most Rust users considered for loops to be a lot cleaner, and adding variety into the code style to please a minority of users is the path to many bad ways of doing each task. It means that no one will be happy with Rust.

@thestinger
Copy link

@blaenk: Your code example is just deliberately making the for loop look more verbose. Nothing stops you from avoiding a temporary in both cases while preserving a clean coding style.

@blaenk
Copy link
Contributor

blaenk commented Jan 16, 2015

@strcat: It's not some sleight of hand attempt to deceive anyone to show that the temporary is necessary; everyone knows the temporary isn't necessary. I realized I should have changed or clarified it for this very reason but I had to go AFK before I got a chance.

The reason I separated it out in the example was because that's what I normally do when the chain gets very long, though in that example it's not particularly long. When the chain is very long I like to have each segment on its own line for readability, and as a result it makes the for loop look very weird, so I separate it out that way.

On Jan 15, 2015, at 4:26 PM, Daniel Micay notifications@github.com wrote:

@blaenk: Your code example is just deliberately making the for loop look more verbose. Nothing stops you from avoiding a temporary in both cases while preserving a clean coding style.


Reply to this email directly or view it on GitHub.

@thestinger
Copy link

It looks perfectly fine without a temporary when following the "official" Rust coding style.

@mneumann
Copy link

@thestinger: when it fits in one line, it's ok:

for x in xs.iter().take(5).filter(|x| x.is_thing()).map(|x| x + 1) {
  // do side effecting operation
  println!("{:?}", x);
}

But if you want to break it up, line by line, it's getting pretty ugly IMHO and rightward:

for x in xs.iter()
           .take(5)
           .filter(|x| x.is_thing())
           .map(|x| x + 1) {
  // do side effecting operation
  println!("{:?}", x);
}

From the visual point of view, and that's my subjective opinion, having an each method whatever it's name might be, be it foreach, exec or do, is much more attractive. People coming from Ruby will just miss it, because there you have both for and each and both are useful on it's own (e.g. each often being used when it fits in one line, while for for multi-line code).

@Ericson2314
Copy link
Contributor

Does iter.map(|x| ...).skip_while(|_| true) achieve the same thing?

@ftxqxd
Copy link
Contributor

ftxqxd commented Jan 16, 2015

Even shorter is iter.map(|x| ...).count(), which is the same as foreach but returns the number of iterations as well (which can easily be ignored).

@nagisa
Copy link
Member

nagisa commented Jan 18, 2015

@Ericson2314 @P1start I mentioned on the discuss thread, that

iterator.fold((), |_, x|{ 
    // for_each body;
})

is equivalent to proposed function. It is also much clearer in intent compared to .map(…).anything().

@huonw
Copy link
Member

huonw commented Jan 22, 2015

FWIW, there's also all (which is exactly the same as the old advance) and any but their names are less descriptive.

@pnkfelix pnkfelix self-assigned this Jan 23, 2015
@pnkfelix
Copy link
Member

pnkfelix commented Feb 5, 2015

So, it seems like debate has mostly settled down at this point. (From what I can tell, we have two camps: One saying that a method like this is worthwhile since its name signals "my callback carries a side-effecting operation", and the other saying "you should just be writing this as a for-loop, not pure method composition." Well, I guess there's also a third camp, perhaps a subcamp of the second, that says "if you really want to do this without making a for-loop, you can use pre-existing methods for it"; but that is not really a counter-argument since the first camp sees intrinsic value in the name for_each.)

Regarding @SimonSapin 's point about Cargo: I believe the core team agrees with him, and wants to encourage things that can go into third party libraries to actually go up on Cargo, regardless of whether they are tiny/trivial things. (The main reason not to put this up on Cargo is if we wanted to encourage people to use it, but I suspect that in general we want to encourage people to use for-loops instead, all other things being equal.)


The RFC seems to have been abandoned by its author, since they have not responded to requests from the community for the addition of a motivating example to the RFC itself.

@larroy am I mistaken about this?

@larroy
Copy link
Author

larroy commented Feb 18, 2015

Something similar has been added to itertools http://bluss.github.io/rust-itertools/doc/itertools/trait.Itertools.html

@pnkfelix
Copy link
Member

Ah, great! and itertools is up on cargo.

Here's the method itself: http://bluss.github.io/rust-itertools/doc/itertools/trait.Itertools.html#method.foreach

@larroy you said "similar"; are the semantics of the above method different than what you proposed here, to your knowledge?

In any case, I'm inclined at this point to close this ticket.

@larroy
Copy link
Author

larroy commented Feb 24, 2015

Well, if there's no intention to put it in rust then we can close as it's already in itertools for those who care.

@aturon
Copy link
Member

aturon commented Mar 5, 2015

As per the most recent discussion, I'm going to go ahead and close this RFC.

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.