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

Add support for Containers #4

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

rprospero
Copy link

This PR adds support for most of the containers in the containers library. Since containers is distributed with GHC, I was hoping it wouldn't be a particularly onerous dependency. I've used this in production and it does properly handle nested structures (eq. Map a (Map b (Map c value))). I've also included some tests to checking these implementations. The tests do included extra dependencies, but those shouldn't be needed for the base library.

There's a couple of bits I can already see being issues:

  1. I left the Torsor module as is an simply added my own module for the collections. It might be better to make a basic Torsor.Types module that Torsor.Containers can import and have Torsor export both. This would remove the need to the user to explicitly import Torsor.Containers to get the orphan container instances.

  2. I'm not great at naming things, so some of the types or function may need new names

  3. I have intentionally not included an implementation for the list type, since diffing will always hang on an infinite list. That's also the reason that the ContainerDiff type uses a Seq - to enforce that the list is finite. However, using lists may be more performant for our use case. Also, lists are a fairly common data structure and I don't believe that any developer would be surprised that they couldn't find the difference between two infinte lists and would not consider this a bug.

  4. I've bumped the version number up to 0.1.1, since everything is backward compatible, but I could also understand this being version 0.2

  5. There is no instance for Tree or Either at the moment. These will hopefully come into a future PR, but they're non-trivial.

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.

1 participant