search

Gabriella439 / pipes

Compositional pipelines
BSD 3-Clause "New" or "Revised" License
489 stars 72 forks source link

Prelude.Lift benchmarks feedback. #64

Closed archblob closed 11 years ago

archblob commented 11 years ago

The code looks kind of messy to me right now but I did quite a few refactorings and I'll have to come back in a couple of hours and look at it with fresh eyes to see what I could have done better.

I lift the composed action so that the benchmarks run faster. Writer overflows as you said it would.

Please take a look when you have the time, and tell me if it's this what you had in mind.

Gabriella439 commented 11 years ago

I left some comments on various lines, but overall it looks very good. Whenever you think it's ready I'd be happy to merge it.

archblob commented 11 years ago

Somewhere the definition of chain changed and I did not notice or the benchmark was wrong all along, but I pushed a fix.

The explicit forall was there as an exercise to remind myself that it really is there all the time, implicitly :-P, but I just forgot to edit it out.

I don't really use this bracketing style but it looked clearer to me in this case, and when I used it the other benchmark module.

I will amend the commit and push the new version.

Gabriella439 commented 11 years ago

Yeah, I changed chain to require a () type to prevent discarding useful return values, which is why you got the error using chain.

archblob commented 11 years ago

If you are ok with this, it can be merged.

Gabriella439 commented 11 years ago

Alright, I will merge it, then. Thank you very much for contributing this! :)

archblob commented 11 years ago

I looked in the commits since I last fetched and I did not find the commit that changed chain so I was a little puzzled until I dropped into ghci and checked the type.

Gabriella439 commented 11 years ago

It was this commit:

30a94ddc0a96b76d8b3de551cf86e76c7eb6cd15