search

snoyberg / mono-traversable

Type classes for mapping, folding, and traversing monomorphic containers
155 stars 67 forks source link

Fix #244. Instances for Data.Vector.Strict #247

Open jcmartin opened 1 week ago

jcmartin commented 1 week ago

This addresses the lack of instances for Data.Vector.Strict that were added in vector-0.13.2.0.

Tests have been added for the new instances and have passed for the following stackage releases:

@BebeSparkelSparkel

@Bodigrim This should help resolve https://github.com/mgsloan/store/issues/179

BebeSparkelSparkel commented 6 days ago

@jcmartin Thanks for the PR!

Requests:

  1. Related to #246, could you add a date to the documentation of the instance when the cpp should be removed. 8 years seems a bit much though perhaps 4? Let me know what you think? Or, perhaps a version of ghc release?
  2. Can you add a warning in the tests indicating that the strict version is not being tested?
BebeSparkelSparkel commented 6 days ago

Also, I have created the mono-traversable-1.1.0.0 branch for breaking changes. Perhaps, this would be better included there so the CPP is not required.

Or perhaps, you want this in 1.0 and the cpp could be removed in 1.1

Let me know what you think.

jcmartin commented 5 days ago

There are many ways to make a decision as to when to create a breaking change. I think the most important one is if you have an important feature or enhancement that is not reasonably possible without a newer version of a dependency. I don't consider adding instances to be a significant enough enhancement to create work for people.

Given that mono-traversable is directly and indirectly a dependency for lots of packages, it would make sense to take a conservative approach to this. GHC 9.4 is currently the "recommended" version and going on what I have heard from in the community GHC 9.4 and 9.6 are where most people are at this point.

Using the stackage releases as a guide, the first distribution to include the new vector library is ghc-9.8.3 in nightly. We could lose backwards compatibility once ghc-9.8.3 or higher has a significant "market share". Maybe a good rule of thumb should be providing support for the last three major releases available on stackage.

With all of this said, I would say we could require vector-0.13.2.0 or higher once ghc-9.12 is in nightly, or there is a significant change in mono-traversable warranting the need for newer dependencies.

@BebeSparkelSparkel What do you think?

jcmartin commented 5 days ago

Also, the strict version is being tested if a new enough version of vector is included. Are you suggesting to include a message when the strict version is not being tested?

BebeSparkelSparkel commented 5 days ago

@gregwebs What do you think about removing the CPP as suggested above?

@jcmartin Yes, have a warning message in a CPP else.

jcmartin commented 4 days ago

@BebeSparkelSparkel This is done.