Strange type rigidity in unlines and unwords

[kozross]

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]

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]

looks good

[snoyberg]

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

• unlines and unwords are class methods, so any change to their type (even just generalizing) is a breaking change
• Since in most cases we're currently just INLINEing a definition from elsewhere, introducing a conversion to list could be a performance hit

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

[kozross]

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]

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]

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]

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 [email protected] 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
#111 (comment)
.

[kozross]

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]

Sounds good!

On Mon, Jan 25, 2016, 5:17 PM Koz Ross [email protected] 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
#111 (comment)
.