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

Delete And(Proc, ...) and introduce ForEach(Proc, ...) #1226

Closed
victornoel opened this issue Nov 4, 2019 · 24 comments
Closed

Delete And(Proc, ...) and introduce ForEach(Proc, ...) #1226

victornoel opened this issue Nov 4, 2019 · 24 comments
Labels

Comments

@victornoel
Copy link
Collaborator

victornoel commented Nov 4, 2019

The And class has a constructor taking a Proc and behaving in:

  • a specific manner: it's not about a logical and
  • and an obscure manner: if you pass a lambda that returns true you can get an unexpected behaviour thinking you are getting the Proc one while you get the Scalar one

This makes bugs happens, such as #1212.

I think we should:

  • just delete it to avoid problems
  • introduce a class ForEach(Proc, Iterable) that implements Proc if one wants to execute something on all elements of an Iterable.

See also #1215 (comment) for a discussion on this.

@paulodamaso
Copy link
Contributor

@victornoel This bug shows us that And is bloated with more than one behavior. But if we are removing this behavior from And, we should provide it somewhere else.

@paulodamaso
Copy link
Contributor

@yegor256 What do you think about this? How about leaving And just for logical conjunctions and implement another class for foreach behavior?

@victornoel
Copy link
Collaborator Author

@paulodamaso I believe Iterable already introduces the behaviour of forEach: indeed it is its "raison d'être" :)

@paulodamaso
Copy link
Contributor

@victornoel I'm talking about an declarative way of iterating through the collection, just like And(Proc, Iterable) behaves

@victornoel
Copy link
Collaborator Author

@paulodamaso I understand, here is my thinking:

  1. foreach behaviour is a procedural behaviour
  2. declarative abstractions exists to be composed (if not you don't need an abstraction)
  3. Proc is the abstraction for composing procedural behaviour
  4. () -> iterable.forEach(xxx) is an instance of Proc structurally

So the only thing we can do is introduce a ForEach class that implements Proc and that takes an Iterable and a Proc and call iterable.forEach(proc::exec).

Why not, but is it really necessary?

@paulodamaso
Copy link
Contributor

@victornoel With this, we could remove the side effect from And and keep the declarative forEach construct in cactoos

@victornoel
Copy link
Collaborator Author

@paulodamaso by "with this", do you mean "introducing ForEach class"?

If yes I think it's answering the original needs in this ticket, I can update the description if you want.

@paulodamaso
Copy link
Contributor

@victornoel Yes, I think that we should implement something like you proposed

@victornoel victornoel changed the title Delete And(Proc, ...) Delete And(Proc, ...) and introduce ForEach(Proc, ...) Dec 4, 2019
@victornoel
Copy link
Collaborator Author

@paulodamaso I've updated the description

@paulodamaso
Copy link
Contributor

@victornoel Thank you

@paulodamaso
Copy link
Contributor

@0crat in

@0crat 0crat added the scope label Dec 5, 2019
@0crat
Copy link
Collaborator

0crat commented Dec 5, 2019

@0crat in (here)

@paulodamaso Job #1226 is now in scope, role is DEV

@0crat
Copy link
Collaborator

0crat commented Dec 5, 2019

Bug was reported, see §29: +15 point(s) just awarded to @victornoel/z

@0crat
Copy link
Collaborator

0crat commented Dec 5, 2019

The job #1226 assigned to @marceloamadeu/z, here is why; the budget is 30 minutes, see §4; please, read §8 and §9; if the task is not clear, read this and this; there will be no monetary reward for this job

@yegor256
Copy link
Owner

@paulodamaso yes, I agree, let's create Foreach class.

@yegor256
Copy link
Owner

@paulodamaso but I think it should just be a wrapper for And

@victornoel
Copy link
Collaborator Author

@yegor256 but do we agree we should still remove the Proc based constructor from And? It's too ambiguous to use I think.

@yegor256
Copy link
Owner

@victornoel yes, I agree to remove

@paulodamaso
Copy link
Contributor

@0crat in

@0crat
Copy link
Collaborator

0crat commented Dec 27, 2019

@0crat in (here)

@paulodamaso Job #1226 is already in scope

ryoku added a commit to ryoku/cactoos that referenced this issue Jan 5, 2020
ryoku added a commit to ryoku/cactoos that referenced this issue Jan 5, 2020
ryoku added a commit to ryoku/cactoos that referenced this issue Jan 9, 2020
ryoku added a commit to ryoku/cactoos that referenced this issue Jan 9, 2020
@0crat
Copy link
Collaborator

0crat commented Jan 10, 2020

The job #1226 assigned to @Roman-Kuvaldin/z, here is why; the budget is 30 minutes, see §4; please, read §8 and §9; if the task is not clear, read this and this; there will be no monetary reward for this job

@0crat
Copy link
Collaborator

0crat commented Jul 5, 2020

The architect of the project has changed; @paulodamaso/z is not at this role anymore; @victornoel/z is the architect now

@victornoel
Copy link
Collaborator Author

@ryoku thank you for the PR

@0crat
Copy link
Collaborator

0crat commented Jul 5, 2020

Job gh:yegor256/cactoos#1226 is not assigned, can't get performer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants