search

ekmett / machines

Networks of composable stream transducers
Other
339 stars 46 forks source link

Generalise the Data.Machine.Group module #97

Closed bens closed 6 years ago

bens commented 6 years ago

I wanted to stream some data to sequentially named files without having to load everything for each file into memory at a time. This patch lets us group inputs with a parameter, like a counter, which makes it easy to stream groups of data that is related.

The old functions I've changed to have a foo_ suffix that fits existing conventions, but I've changed types of existing functions so that would cause a major version bump. If there are other good names that avoid that I'm interested.

bens commented 6 years ago

Bump?

bens commented 6 years ago

I was wondering if there's anything holding this up or if it's unsuitable somehow? I haven't had any comments on the last commit I pushed. Cheers.

acowley commented 6 years ago

I think it sounds good, and the code looks good. I imagine nobody’s jumped in to merge because the compatibility break is unfortunate. My opinion is that the breakage is okay, but I think it would also be fine to stick the generalized versions in another module that users can import if wanted.

Hopefully others can chime in on if they’re concerned about the breakage.

bens commented 6 years ago

If it were to go into a separate module, what would be a good module name? Data.Machine.Group.General, Data.Machine.Group', or Data.Machine.Split maybe? Nobody seems particularly interested in the compatibility break so far.

bens commented 6 years ago

I've picked Data.Machine.Group', the Data.Machine.Group module is untouched.

bens commented 6 years ago

I'd love a resolution to this, is there anything that I need to do to get it merged or is it just unsuitable? Thanks.

bens commented 6 years ago

I've changed the old D.M.Group module to just call the D.M.Group' functions. The tests still pass.

YoEight commented 6 years ago

I think the code is good too. My only concern is about your Group' module name. I suggest you rename it D.M.Group.Final as I saw @ekmett using that pattern on his free package when having a data structure with different encodings. For example Control.Applicative.Free has Fast and Final sub-modules. I think that pattern could be used here.

I will merge that MR right after :-)

bens commented 6 years ago

How aboout D.M.Group.General? This isn't a different encoding, I'm just generalising and providing some more specialised versions of existing functions, Group' seemed reasonable because it supersedes the old Group module and I hope this one takes over the Group name at the next major version.

YoEight commented 6 years ago

I’m fine with D.M.Group.General

On 1 Aug 2018, at 14:20, Ben Sinclair notifications@github.com wrote:

How aboout D.M.Group.General? This isn't a different encoding, I'm just generalising and providing some more specialised versions of existing functions, Group' seemed reasonable because it supersedes the old Group module and I hope this one takes over the Group name at the next major version.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

YoEight commented 6 years ago

It seems you added a commit after our discussion but Group' name is still here. Did your rebase go wrong?

bens commented 6 years ago

I've moved it to D.M.Group.General, thanks YoEight!

YoEight commented 6 years ago

Thanks for your work @bens !