rust-itertools / itertools

Extra iterator adaptors, iterator methods, free functions, and macros.
https://docs.rs/itertools/
Apache License 2.0
2.69k stars 303 forks source link

Add macro `iunpack` #841

Closed A4-Tacks closed 8 months ago

A4-Tacks commented 8 months ago

Add it, simply and quickly unpack iterators to patterns.

phimuemue commented 8 months ago

Hi there, this is some impressive macro-programming.

But from a user's perspective: Is this really better than a manual collect_tuple (and its cousins), or - in the special cases - manually collecting the iterator into separate variables? iunpack requires the user to learn yet another mini-language (see the *, **, *c=, etc). We could justify this if the macro covered all use cases - but e.g. can it e.g. unpack into a HashSet, but only values that match the pattern 1 | 2? And if so, how easy would it be to specify this?

I think itertools should provide (relatively) fool-proof and intuitive helpers, and I don't think iunpack fits this category. So my gut feeling would be to decline this PR.

A4-Tacks commented 8 months ago

Hi there, this is some impressive macro-programming.

But from a user's perspective: Is this really better than a manual collect_tuple (and its cousins), or - in the special cases - manually collecting the iterator into separate variables? iunpack requires the user to learn yet another mini-language (see the *, **, *c=, etc). We could justify this if the macro covered all use cases - but e.g. can it e.g. unpack into a HashSet, but only values that match the pattern 1 | 2? And if so, how easy would it be to specify this?

I think itertools should provide (relatively) fool-proof and intuitive helpers, and I don't think iunpack fits this category. So my gut feeling would be to decline this PR.

I think it's easy to understand, and we have macro expand, right? Its operation is like extending the slice pattern to iterators

A4-Tacks commented 8 months ago

For example, it can unpack structures or verify the correctness of enumerations when unpacking iterators, which will be extremely convenient and can simplify many codes

A4-Tacks commented 8 months ago

And this macro implementation can effectively handle iterators with variable numbers of elements for quick single branch matching at the beginning and end, without the need for lengthy code. One of the powerful features of this macro is the use of buffers to rely solely on one-way iterators to match tails

Philippe-Cholet commented 8 months ago

It seems powerful but quite unusual at first sight, and all those loop-break around the macro (but I guess it could be presented differently). I get that you understand everything here as you made it but it's not that easy. I would agree with phimuemue.

After looking at the internals, I need to say that we currently use stable Rust 1.43.1 and "let-else" is not available for us yet (and 1.65 is not for tomorrow).

Honestly after all the debate of iproduct!(something; n) (#264) that should be simple enough (which I intend to solve in a near future), this definitely seems on another level.

A4-Tacks commented 8 months ago

It seems powerful but quite unusual at first sight, and all those loop-break around the macro (but I guess it could be presented differently). I get that you understand everything here as you made it but it's not that easy. I would agree with phimuemue.

After looking at the internals, I need to say that we currently use stable Rust 1.43.1 and "let-else" is not available for us yet (and 1.65 is not for tomorrow).

Honestly after all the debate of iproduct!(something; n) (#264) that should be simple enough (which I intend to solve in a near future), this definitely seems on another level.

Regarding the issue of let-else, may it be possible to modify it to an internal code block? I still hope it will be merged.

A4-Tacks commented 8 months ago

It seems powerful but quite unusual at first sight, and all those loop-break around the macro (but I guess it could be presented differently). I get that you understand everything here as you made it but it's not that easy. I would agree with phimuemue.

After looking at the internals, I need to say that we currently use stable Rust 1.43.1 and "let-else" is not available for us yet (and 1.65 is not for tomorrow).

Honestly after all the debate of iproduct!(something; n) (#264) that should be simple enough (which I intend to solve in a near future), this definitely seems on another level.

Having changed it to use if-let, is it available now?

A4-Tacks commented 8 months ago

Can it run now? Can it be merged?

Philippe-Cholet commented 8 months ago

I just let our CI run, it passed (thanks to the removed "let-else") which does not mean it'll be merged.

The "merge process" might be considered slow for new additions and especially for something that is not easy like this. But if we add something, we commit ourselves to maintain it. I'd first like to know if the changes change @phimuemue's opinion on this as it seems a bit more friendly than initially.

phimuemue commented 8 months ago

I just let our CI run, it passed (thanks to the removed "let-else") which does not mean it'll be merged.

The "merge process" might be considered slow for new additions and especially for something that is not easy like this. But if we add something, we commit ourselves to maintain it. I'd first like to know if the changes change @phimuemue's opinion on this as it seems a bit more friendly than initially.

I hope I do not come across as rude, but this did not change my opinion. Imho this macro - while impressive - has downsides:

I think this has its place in a code-golf/AoC-like crate, but not in itertools. But I won't veto if @Philippe-Cholet or @jswrenn wants to merge.

scottmcm commented 8 months ago

iunpack requires the user to learn yet another mini-language

I think this is the biggest things that, personally, pushes me away from saying this is a good fit for itertools.

The other macros that exist are about handling a variable number of heterogeneous parameters, where macros are the only reasonable option today, but which act far more like functions. (chain!(a, b, c) could be made to work as multichain((a, b, c)) with some traits implemented on tuples, for example, instead.)

But iunpack is a DSL, which feels different enough from the other things in here that I think it'd be better in a different crate.

Philippe-Cholet commented 8 months ago

I think we three agree that it is a nice and powerful macro that does not really fit our crate. It should belong somewhere, maybe its own crate, as I do not know which crate it could be a good fit.

phimuemue commented 8 months ago

Closing, outside the scope of itertools.