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 timeline packages #1

Merged
merged 24 commits into from
Mar 15, 2023
Merged

Add timeline packages #1

merged 24 commits into from
Mar 15, 2023

Conversation

kokobd
Copy link
Contributor

@kokobd kokobd commented Mar 6, 2023

TODOs

  • improve comments and descriptions
  • remove dependency on string-interpolate
  • test if it works across all supported ghc versions
  • add proper version bounds

Copy link
Member

@JackKelly-Bellroy JackKelly-Bellroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For simple packages like timeline and aws-arn, I think that we are better off using nixpkgs rather than haskell.nix.

Copy link
Member

@JackKelly-Bellroy JackKelly-Bellroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many nits, but this is looking good.

timeline-hedgehog/src/Data/Timeline/Hedgehog.hs Outdated Show resolved Hide resolved
timeline-hedgehog/timeline-hedgehog.cabal Outdated Show resolved Hide resolved
timeline-tests/test/Spec/Data/Timeline.hs Outdated Show resolved Hide resolved
timeline-tests/timeline-tests.cabal Outdated Show resolved Hide resolved
timeline/src/Data/Timeline.hs Outdated Show resolved Hide resolved
timeline/src/Data/Timeline/Internal.hs Outdated Show resolved Hide resolved
timeline/src/Data/Timeline/Internal.hs Outdated Show resolved Hide resolved
timeline/src/Data/Timeline/Internal.hs Outdated Show resolved Hide resolved
timeline/timeline.cabal Outdated Show resolved Hide resolved
timeline/src/Data/Timeline/Internal.hs Outdated Show resolved Hide resolved
flake.nix Outdated Show resolved Hide resolved
Copy link
Contributor

@lrworth lrworth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this package need to be based on time? It seems like any totally-ordered type would work.

Could we call this continuous-map or something and remove the dependency on time?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
timeline/src/Data/Timeline.hs Outdated Show resolved Hide resolved
timeline/src/Data/Timeline/Internal.hs Outdated Show resolved Hide resolved
@JackKelly-Bellroy
Copy link
Member

Does this package need to be based on time? It seems like any totally-ordered type would work.
Could we call this continuous-map or something and remove the dependency on time?

Yeah I think if you conceptualise it as a map of non-overlapping intervals but then say in the docs "one common application is timeline data", you get a much more flexible data structure.

Something to consider: Do we want to require that the set of intervals completely covers the range of the key type? Certainly in the timeline case we might want to require that, but an interval map with a default value represents a constant function, and then you can quite reasonably build on top of that.

@kokobd
Copy link
Contributor Author

kokobd commented Mar 14, 2023

It's easy to replace UTCTime with Ord t, I'll do that. But I don't think we should strive to give it a more abstract name now. The original motivation for creating a timeline package is to represent "timelines", and the interval map is just one representation of that. We may support more representations, like pure functions, in the future.

In the long run, IMO, continuous-map is a standalone package that does its job, and timeline is a package built on top of that, to provide different timeline representations.

@kokobd
Copy link
Contributor Author

kokobd commented Mar 15, 2023

Merging. If we have anything else to change, let's do that in another PR.

@kokobd kokobd merged commit 4db818e into master Mar 15, 2023
@kokobd kokobd deleted the initial branch March 15, 2023 00:40
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.

3 participants