tfausak / octane

:rocket: Parse Rocket League replays.
https://www.stackage.org/nightly/package/octane
Other
39 stars 1 forks source link

Overloaded labels #44

Closed tfausak closed 8 years ago

tfausak commented 8 years ago

This pull request explores using OverloadedLabels instead of DuplicateRecordFields. One problem I keep running into with duplicate record fields is that Octane.unpack x is too hard to use. It basically never infers the correct type, even with explicit signatures. I end up importing the type I want to use and calling it directly, which is actually more annoying than just using prefixed field names.

By switching to overloaded labels I can write code like #unpack x. That will work as long as GHC knows the type of x, which it usually does.

Another benefit of this change is that the documentation will be better. Haddock doesn't like duplicate record fields, so they are broken. See the Replay documentation for an example.

I am aware of two downsides to this approach. One, it requires a lot more language extensions. That's not really a problem per se, but it can be kind of annoying. (In particular, I end up using ScopedTypeVariables a lot more to explicitly give the type.) Two, I've heard that it can slow down the build. Since I have a lot of records, that might be bad. But I'll have to run a benchmark at the end to see if it has any effect, either at compile or run time.

tfausak commented 8 years ago

+827 −564

The additions are mostly language pragmas and imports. It might be worthwhile to enable some language extensions at the package level. And create a package-local prelude to avoid importing all the same stuff.

tfausak commented 8 years ago

After these changes, the build is about 30% slower.

# main
octane> git checkout 25d557f45a348d2a4fe102078dab33d8bcf6592d; and stack clean --full; and time stack build
...
       35.90 real        31.20 user         3.31 sys

# overloaded-labels
octane> git checkout f2d311d93095386b12874b13448c8c4db1065340; and stack clean --full; and time stack build
...
       46.97 real        42.21 user         4.43 sys
tfausak commented 8 years ago

Unfortunately doctest does not support package-level language extensions. See https://github.com/sol/doctest/blob/0.11.0/README.markdown#using-ghc-extensions.

tfausak commented 8 years ago

This pull request is quickly getting out of control. It should probably be a few separate ones.

tfausak commented 8 years ago

Now that I've done all this, I can't help but wonder... Is it worth it? Yeah, I removed a bunch of lines of pragmas and imports. But is my code easier to understand and/or develop? I'm not sure. Especially since I tend toward name spaced imports everywhere, just seeing Bimap in the middle of a module, it's hard to tell where it came from. And sure, there isn't that much in the Basics module. But I like being able to look at any given module more or less in isolation and figure out where everything is coming from.

So even though I can drop about 600 lines of code, I don't think it's an improvement. I could drop even more lines by importing everything into Basics and using that everywhere! Obviously that's not good.

tfausak commented 8 years ago

Closing in favor of #45.