Reworking `NonEmpty`

[fosskers]

This PR builds off of #174 , etc., to:

• actually expose the RIO.NonEmpty.* modules
• correct some minor documentation issues
• expose the (:|) operator after all

Previous work had purposefully left out (:|), citing conflicts with other packages. However, since NonEmpty is from base and the cited conflicts are not, I believe that the NonEmpty version should be considered the "real" one. Note that the vanilla List type itself and its constructors are always exposed regardless of NoImplicitPrelude, and so we could argue that its twin (NonEmpty) should be exposed in the same way.

I'm willing to reconsider this stance on (:|) of course, and have left its addition as the final commit in this PR, to aid a rollback.

[fosskers]

The currently released version on Hackage is 0.1.9.2, and the version marked in package.yaml was already 0.1.10.0, so no update needed there. I did tweak the CHANGELOG to mention (:|).

[akhra]

Oh dear, I should have read this more closely...

Previous work had purposefully left out (:|), citing conflicts with other packages. However, since NonEmpty is from base and the cited conflicts are not, I believe that the NonEmpty version should be considered the "real" one. Note that the vanilla List type itself and its constructors are always exposed regardless of NoImplicitPrelude, and so we could argue that its twin (NonEmpty) should be exposed in the same way.

I don't think this argument holds. Here was my reasoning for excluding (:|).

1. We explicitly excluded several List-related functions that are in Prelude from the main RIO module, because they conflict with things that are not in base! Instead, we have RIO.List which is intended for qualified import just like RIO.Map, RIO.Set, etc. The idea here, in my understanding, is that if there needs to be disambiguation in common cases, it's a cognitive burden to have an implicit default.

2. A useful quality to maintain is being as close as possible to a drop-in Prelude replacement, with minimal migration. We've been pretty cautious about adding identifiers to RIO that aren't in Prelude, choosing only those with overwhelming and consistent adoption. (I outright reject the suggestion that because (:) is unavoidable, (:|) must also be; I consider the former to be an unfortunate implementation wart, not a pattern to be followed. The practical reality of several competing (:|)s only reinforces this.)

3. As I see it, (:|) is not the correct interface to NonEmpty! In a similar vein to Map and Set, the better interface is via nonEmpty and toList. Unlike those types, the raw constructor is simple and preserves the necessary invariants, so it was safe to expose; but IMO that's a leaked implementation detail, not an interface that should be encouraged (and inclusion in RIO is encouragement).

Obviously all of this is up for discussion, but that discussion should happen. If we want to enable construction of NonEmptys without only RIO imported, I would much rather see nonEmpty exposed than (:|). It also has conflicts, but there's a much stronger argument for the name of a type being an unsurprising conflict if someone else uses it. NonEmpty.toList can't be exposed, but Foldable.toList already is and works fine.

[fosskers]

Thanks for weighing in. Here are my thoughts (following the same 1-2-3 scheme):

(1) I like that RIO doesn't give List special treatment w.r.t. its common functions (like say interpolate). With the exception of (:|), we're following that pattern with NonEmpty. Everything else about NonEmpty is jailed behind an import.

(2) Note that even with NoImplicitPrelude, the Tuple type and its constructors are also exposed. A user would expect to be able to pattern match on these, as with List. I myself pattern match on (:|) often, and would miss it were it hidden behind an import. One point of RIO is to expose the top-level symbols we always use, to avoid the usual:

import Data.Map.Strict (Map)
import qualified Data.Map.Strict as M

If we're using NonEmpty, we'd likely have:

import RIO  -- Exposes `NonEmpty` type
import qualified RIO.NonEmpty as NEL

and it would be frustrating to have to add import RIO.NonEmpty ((:|)) just to be able to pattern match / construct. Calls like NEL.:| also give me allergic reactions :laughing: In general, I'll stand by the idea that since (:|) is in base, the downstream libs that also expose it have the impetus to adapt.

(3) I think use of (:|) is legitimate in general. Map and Set are different here: it's okay to jail everything about them behind an import, since we can't construct them ourselves anyway, nor pattern match, of course. So, like List and Tuple, I think NonEmpty's (:|) should be considered a special case. Note also that the NonEmpty module exposes no singleton function, leaving pure as the only other way to generally construct one from a single element.

[akhra]

Hm. I still disagree that (:) and (,) being impossible to hide is anything but an implementation accident, but I'm 100% with you on NE.:| being cancer and the extra import to avoid it being a pain. And your (3) is reasonable, right up to the point where you bad-mouth pure. 😉

So how about a compromise: add nonEmpty too. That gives us nonEmpty/pure/(<>)/toList for smart de/construction, which I'm entirely happy with; and (:|) for explicit, which you've convinced me is benign (and @snoyberg already signed off on).

[fosskers]

To be clear, you mean nonEmpty :: [a] -> Maybe (NonEmpty a) ? I could get behind that, if there's a reasonable expectation that a user might want to nonEmpty (unqualified), and then somehow use the result without having import qualified RIO.NonEmpty as NEL in scope.

@snoyberg , any thoughts? I can do a follow-up PR.

[akhra]

To be clear, you mean nonEmpty :: [a] -> Maybe (NonEmpty a) ?

Correct. Plenty can be done with it via typeclasses, and there are also situations where an imported function expects NonEmpty input and we don't locally need to do anything but construct it; compare working with Text and ByteString interfaces via IsString and Monoid.

[snoyberg]

I don't have strong thoughts, I don't personally use NonEmpty that often. I don't have an objection to exporting nonEmpty, it seems benign enough.

[fosskers]

So bet it! I'll fire up a follow-up PR.