search

snoyberg / classy-prelude

A typeclass-based Prelude.
108 stars 15 forks source link

Strange type rigidity in unlines and unwords #111

Open kozross opened 8 years ago

kozross commented 8 years ago

I don't know if this should be in mono-traversable or not, but I figured I should report it. The type of unlines is reported by GHCi as:

unlines :: Textual t => [t] -> t

Is there a reason for such rigidity in the collection type? Near as I can tell, unlines is a special fold, and thus, surely anything Foldable should be game for it? The same goes for unwords - this is also basically a special fold, and thus, surely anything Foldable should be game?

I am very new to Haskell, and don't always understand the motivations behind certain choices, so I could be completely wrong, but this type rigidity strikes me as strange, given the focus on typeclass-level behaviour of classy-prelude.

snoyberg commented 8 years ago

I don't see a problem with this approach, though, in classy-prelude style, it might be more idiomatic as:

unlines :: (MonoFoldable mono, Text (Element mono)) => mono -> Element mono

@gregwebs thoughts?

gregwebs commented 8 years ago

looks good

snoyberg commented 8 years ago

Actually, looking at the code, there are two downsides to making this change:

So I'm less enthusiastic about making the change based on this.

kozross commented 8 years ago

I can understand this reasoning, but I don't think that they should preclude these changes from being made.

The fact that this will be a breaking change is not in-and-of-itself a reason not to do it. Maybe this means it should be delayed to the next major release, but an API break is not a good justification for not doing something IMO.

As far as the performance hit goes, I'm not sure if this will even be visible. We can't really guess at the performance characteristics of code without measuring it first - and even then, it might not matter in most use cases (which we also can't necessarily know for sure).

snoyberg commented 8 years ago

IME, breaking changes are a reason to not do something. Getting a tiny improvement to an API at the cost of forcing people to check for compatibility with an API, and possibly breaking some people's code, is not a good trade-off. The change here is nice, but not vital: using unlines . toList is not that big an overhead.

kozross commented 8 years ago

Well, could it be incorporated into a future breaking change for something less trivial? Given that neither classy-prelude nor mono-traversable are at 1.0 yet, such a change could potentially happen - would you still be opposed in such a case?

snoyberg commented 8 years ago

I'm not ruling anything out, I'm simply pointing out that there's a cost to this change, making it less-than-obvious that we'll do it. This hesitance applies whether or not we're already breaking the API. However, given the unlikeliness of people defining their own Textual instances, I'd be OK with merging a PR for this change to mono-traversable when we make our next major release.

On Mon, Jan 25, 2016 at 1:06 PM, Koz Ross notifications@github.com wrote:

Well, could it be incorporated into a future breaking change for something less trivial? Given that neither classy-prelude nor mono-traversable are at 1.0 yet, such a change could potentially happen - would you still be opposed in such a case?

— Reply to this email directly or view it on GitHub https://github.com/snoyberg/classy-prelude/issues/111#issuecomment-174473062 .

kozross commented 8 years ago

I guess I should make a PR to mono-traversable then? You can merge it (if you decide to do so) whenever it suits you best.

snoyberg commented 8 years ago

Sounds good!

On Mon, Jan 25, 2016, 5:17 PM Koz Ross notifications@github.com wrote:

I guess I should make a PR to mono-traversable then? You can merge it (if you decide to do so) whenever it suits you best.

— Reply to this email directly or view it on GitHub https://github.com/snoyberg/classy-prelude/issues/111#issuecomment-174496060 .