Introducing split'/splitOn' and deprecating split/splitOn

[Aster89]

This PR is somewhat in line with the spirit of #558.

Specifically, like that PR introduced initsNE/tailsNE as variants of inits/tails that return NonEmpty Text instead of [Text] (for both lazy and strict modules), this change is introducing split'/splitOn' as variants of split/splitOn that return NonEmpty Text instead of [Text] (for both lazy and strict modules).

Furthermore, I'm proposing to substitute, at some point in the future, split/splitOn with the split'/splitOn' that I'm introducing now. In this view, the present PR represents step 1 in the following plan:

1. Deprecate split/splitOn and introduce split'/splitOn' that aim to eventually substitute them;
   • additionally, I've also changed split/splitOn's implementations to simply forward call the new split'/splitOn' and turn them NE.toLists; this way existing tests should be enough;
2. let time pass for the community to transition split/splitOn → split'/splitOn';
3. turn split/splitOn into synonyms of split'/splitOn', and deprecate the latter;
4. remove split'/splitOn' for good.

Additionally, I'm also proposing to rename initsNE/tailsNE to inits'/tails', and deprecate all of initsNE/tailsNE/inits/tails, starting the same plan as I proposed for split/splitOn.

---

I've kept the change split in a few commits to make it easier to inspect and, possibly, drop something.

[Bodigrim]

I'm happy to have a NonEmpty counterparts for split and splitOn, but I disagree on renaming initsNE / tailsNE and on deprecating the existing split and splitOn.

While initsNE is not a great name perhaps, I think inits' is not much better and thus is not worth changing. I don't recall other libraries using ' to denote nonemptiness of results.

split and splitOn are not wrong; even while the return type can be refined, many users are perfectly happy with them as is.

Sorry if this sounds discouraging. Unfortunately breaking changes in text are a huge pain for the ecosystem, I'd rather avoid them unless there is a strong justification.

[Aster89]

I'm happy to have a NonEmpty counterparts for split and splitOn

Good.

While initsNE is not a great name perhaps, I think inits' is not much better and thus is not worth changing. I don't recall other libraries using ' to denote nonemptiness of results.

I've renamed my new split' and splitOn' to splitNE and splitOnNE.

but I disagree on renaming initsNE / tailsNE and on deprecating the existing split and splitOn.

Removed that part of the change.

---

split and splitOn are not wrong; even while the return type can be refined

Well, isn't that the same as saying that if head was implemented like in head (x:xs) = [x] it wouldn't have been wrong, even while the type could be refined from [a] -> [a] to [a] -> a?

Unfortunately breaking changes in text are a huge pain for the ecosystem

Isn't the process of deprecating over a long enough time frame a huge reduction of that pain?

[Bodigrim]

It will be the maintainers of text hearing from frustrated users, so it eventually comes to our personal taste and experience. My view is that deprecation and eventual breaking change and subsequent bump of the major version of text and everyone downstream relaxing their version bounds (and adjusting their code, which worked perfectly well before) is not worth it in this particular case.

@Lysxia what do you think?

[Aster89]

I've managed to give a first look at test failures.

Beside the fact that NonEmpty.singleton did not exist before base 4.15, I see a failure I don't understand (for both strict and lazy versions), also beacuse it doesn't happen on GHC 9.14.

splitOnNE is implemented like this:

splitOnNE :: HasCallStack
        => Text
        -- ^ String to split on. If this string is empty, an error
        -- will occur.
        -> Text
        -- ^ Input text.
        -> NonEmptyList.NonEmpty Text
splitOnNE pat@(Text _ _ l) src@(Text arr off len)
    | null pat        = emptyError "splitOnNE"
    …
    …
    …

And to avoid code duplication, I changed the implementation of splitOn to just forward arguments to splitOnNE and call toList on the result, like this:

splitOn pat = NonEmptyList.toList . splitOnNE pat

but this would cause test failures in GHC < 9.14, e.g. for 9.12.2:

        t_splitOn_split:                                                   FAIL
          *** Failed! Falsified (after 1 test):
          ""
          Sqrt {unSqrt = []}
          Exception thrown while showing test case: 'Data.Text.splitOnNE: empty input'
          Use --quickcheck-replay="(SMGen 4338970524477664030 2100411453152539969,0)" to reproduce.
          Use -p '/t_splitOn_split/' to rerun this test only.
        tl_splitOn_split:                                                  FAIL
          *** Failed! Exception: 'Data.Text.Lazy.splitOnNE: empty input' (after 1 test):
          ""
          Sqrt {unSqrt = []}
          Exception thrown while showing test case: 'Data.Text.Lazy.splitOnNE: empty input'
          Use --quickcheck-replay="(SMGen 5739323916416949207 4433519835723925665,0)" to reproduce.
          Use -p '/tl_splitOn_split/' to rerun this test only.

Changing splitOn to

splitOn pat src
    | null pat  = emptyError "splitOn" -- XXX Why if I comment this tests fail?
    | otherwise = NonEmptyList.toList $ splitOnNE pat src

fixes the tests (at least those I've run locally).

I don't understand why, given I am erroring on null pat in splitOnNE.

[Bodigrim]

I think that's because of different laziness properties. Tests evaluate results before comparison, but only to a week normal form. In one case the error is immediate, in another it only pops once you start looking inside NE.toList, I reckon.

[Lysxia]

I think if we added initsNE/tailsNE then we might as well add splitNE/splitOnNE. I also don't think there's anything worth deprecating at this point.

Well, isn't that the same as saying that if head was implemented like in head (x:xs) = [x] it wouldn't have been wrong, even while the type could be refined from [a] -> [a] to [a] -> a?

The situation with split (or inits) is different from head. head is partial and too easy to misuse. In contrast, split itself is a total function, with much more limited use cases to boot. Even if it leads to using head afterwards, at least those uses are justified locally, and it would take some effort before that becomes a problem. Hence a stronger argument is needed to deprecate split.

Tests fail

The tests fail with older base indeed because toList used to be lazy before base 4.22 (see proposal haskell/core-libraries-committee#107), whereas the current (expected and desirable) behavior of splitOn is to fail immediately if the separator is empty.

[phadej]

The additions/changes are wrong.

Addition of examples/doctests should not affect the actual code. There is no OverloadedStrings needs in the implementation, thus the extension doesn't need to be enabled.

Also :seti is there specifically to not interfere with code loading. Don't change.

While I'm not a text maintainer, i'm not happy to see a change with some irrelevant changes. Please cleanup the patch.

[Bodigrim]

Thanks for your work on this issue! I have a few suggestions.

[Bodigrim]

Let's avoid OverloadedStrings if it's only for the sake of "". When working on text, one'd better be explicit what is String and what is Text. You can use a guard with Just (c, cs) | null cs -> ..., for example.

[Lysxia]

These loops should produce a list [Text] instead of NonEmpty Text. Here every go outputs a :| to be immediately consumed by NE.cons.

[Lysxia]

Here too comb should produce [Text], otherwise every comp produces a :| only to be immediately consumed by NE.cons. Similarly the NonEmpty Text argument should be either left as a list or unpacked as two arguments.

[Lysxia]

One more go that should return [Text].