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

Use index instead of shit to avoid changing input #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ruifortes
Copy link

No description provided.

@dominictarr
Copy link
Member

this is a breaking change because it reverses the order that the array is added to the stream.
what is the goal here? is it performance or purity?

@ruifortes
Copy link
Author

Sorry about the order.
In my case the goal was to mutate the source.
Nevertheless should the source preferentially kept intact?

@dominictarr
Copy link
Member

sorry I don't understand. Can you explain what you are using this for and why you need this change?

@ruifortes
Copy link
Author

ok, its not the ideal use of pull-streams but I was iterating a large json object.
For the result list I wanted some filtered and unique results but meanwhile I also wanted to fix some issues in the original json.
So, besides geting the filtered results I also wanted to mutate de source.
Granted it's not the ideal scenario for pull-streams I guess that keeping the input untouch should be prefered to changing it. Unless that's intentional of course.
Is this a bad practice?

@dominictarr
Copy link
Member

Well, if you are gonna do it like that, you should make a pull-stream module that is about that, with tests for that usecase... pull-flatmap is intended to be a functional programming pattern, so it does not approve of mutations like this.

@ruifortes
Copy link
Author

But right now pull-flatmap DOES mutates the input. It zeroes the input arrays.
Does it need to do that to follow the functional programming pattern?
"map" followed by "flatten" doesn't

@dominictarr
Copy link
Member

I mean, the part that is ment to be functional is the function you pass to flatmap.
how flatmap works internally is it's own business, and you probably shouldn't be holding a reference to that array after you return it.
if you did want to mutate the array though... it seems to me my way might work better because at least you can inspect the array and see what hasn't been emitted from the stream yet, on the other hand, if it used an index you wouldn't know if you were adding an item that was still gonna be in the stream or not.

Anyway, I'm not gonna merge this because it's a unnecessary breaking change.

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.

2 participants