haskell-hvr / uuid

A Haskell library for creating, printing and parsing UUIDs
http://hackage.haskell.org/package/uuid
61 stars 38 forks source link

Split UUID data type out into separate Cabal package #7

Closed BardurArantsson closed 9 years ago

BardurArantsson commented 9 years ago

(STATUS: GOOD TO GO . See latest comment)

It includes a few of the commits from my "Cleanups" pull request (which should hopefully be uncontroversial), just as a matter of convenience. The diff might be easier to scan if you choose to merge that pull request first.

Anyway, this represents the initial stab at splitting the data type definitions from the UUID-generation code.

The gist of the changes is:

What remains:

Splitting out tests/benchmarks which are relevant to Data.UUID.Types into their own separate tests. To be certain that this is done correctly, I'd like to start actually splitting things into separate .cabal files, so I need to know how you'd like the code organized -- in two separate git repositories or in two separate directories in a single repository? (As mentioned by e-mail, I think I'd prefer the latter since it's much easier to co-evolve code that way.)

I'd also like to get rid of the binary dependency from Data.UUID.Types, but I don't see any good way of doing it which doesn't require orphan instances. Any ideas? (I think I would propose just living with orphan instances and adding them directly to Data.UUID.UUID.) This isn't an insurmountable problem for my use case, but "binary" also drags in "containers", and I don't see any particular reason to give "binary" privileged status over other serialization approaches. (Interestingly, "binary" also doesn't appear to be in the Haskell Platform, so there's that.)

Any and all comments welcome :).

BardurArantsson commented 9 years ago

Hum, I see that the diff wasn't updated -- I'll just do a quick force push to force github to update the diff.

BardurArantsson commented 9 years ago

There we go. Diff looks a lot saner now :).

BardurArantsson commented 9 years ago

I gather there are no strong opinions on how to proceed? I think I'll try to do the two-packages-in-single-repository approach. (It can always be split out later. I had feared that the github diffs would be mangled by renames, but that doesn't appear to be the case.)

I'd also like ideas on the binary instance. (I'll obviously keep it in the uuid-types package initially.) I guess one valid approach might be to actually have uuid wrap the type exported by uuid-types in its own newtype, but that seems a bit icky...

BardurArantsson commented 9 years ago

OK, so here are the fully split packages. Notes:

What remains:

BardurArantsson commented 9 years ago

Btw, gitk is a better at showing the diffs since it also detects "copy + modify", which I'm not sure github does.

aslatter commented 9 years ago

To answer many of your questions/concerns/&c.:

At some point I'll add big, scary warnings to Type.Internal telling folks not to use it, and also mark it "not home" for Haddocks, but that's also something I can merge without having up front.

BardurArantsson commented 9 years ago

OK, thanks for the commentary :). I'll try to get Travis working and we'll see where we stand. (Almost everything else can at the very least be deferred to later.)

BardurArantsson commented 9 years ago

OK, so I think I have the more-or-less the final version now. See the commit comment for e180cfe + diff (best viewed in gitk) for the final set of notes.

AFAICT this hasn't broken API compatibility, so I chose to bump "uuid" to 1.3.9. Perhaps we should be conservative about it and bump to 2.0.0, I'm not sure.

I did a few experiments with Travis, and arrived at the current solution as the most CPU-efficient. There's another option where more things are parallelized (via sandboxing and parallel builds for each of the two packages and "env:" in the Travis config), but in practice it seemed a lot slower, probably due to repeated downloading/compilation -- I think this is something we can revisit later if need be.

Let me know if there's anything that needs to be changed before merge... assuming the Travis build succeeds :)

BardurArantsson commented 9 years ago

Hum, playing around with using this from a different project, it strikes me that Data.UUID.Types.UUID for the main module in uuid-types looks a bit silly for a module name. I think I'll change it to Data.UUID.Types, so please hold off on merging for now. I'll probably get to this in a day or two.

aslatter commented 9 years ago

That seems reasonable to me.

BardurArantsson commented 9 years ago

Done.

BardurArantsson commented 9 years ago

Any remaining issues to you want taken care of before merging?

aslatter commented 9 years ago

Sorry for the delay on the merge! Everything looks okay, and the generated docs look good, too.

BardurArantsson commented 9 years ago

Great, thanks!