Remove implicit space parsing from combinators

[timjs]

Hi @Punie!

I like this package contains some classical combinators as some, many and between. However, they hardcode items should be separated by spaces. In the most general case, this is not needed. I propose the following changes, implemented by this pull request:

• many and some do not implicitly parse spaces any more
• use spacy to parse spaces around a parser
• use by to parse items separated by something
• use for the old behaviour use: some_old item == by spaces item

Also, to be consistent with other extra-packages, like those published by elm-community, the module exporting these functions should be called Parser.Extra instead of Parser.Extras. Above proposed changed imply a semantic change, and therefore a major bump, I also changed the name of the module by removing the plural "s".

What do you think of these changes?

[Punie]

Hi @timjs !

First of all, thank you for your feedback.

I think renaming Parser.Extras to Parser.Extra should be a no-brainer (I don't know why but I was sure -- at least in my head -- that it was x.Extras in other packages. Oh well, I might have been drunk that day).
However, in the long run, it might mean we should rename the package to elm-parser-extra and that's not so easily done.

I also have some refactoring / cleaning-up planned over the next week-end so I might wait until then to review and merge your changes. (I hope it's not too inconvenient for you to wait until then for a new release).

[Punie]

Having some return a Parser (a, List a) was very much intentional as to retain type safety.

Basically, (a, List a) represents a type-safe non-empty list which enables the use of some safe versions of head, tail or even a foldl/foldr with no need for an initial value.

If there is a use case for some returning only a List a, I guess we could implement it separately.

[Punie]

I reviewed some of your changes and made a few comments.

I'm also in the process of writing test cases for every exposed function in those modules and adding a travis.ci config, so I would advise to wait for these to hit and then update your branch.

[Punie]

Alright, I added the Travis CI integration as well as (some) tests.

@timjs could you please rebase your branch on top of master before making changes and verifying that tests still pass ?

[timjs]

Yes I’ll do that! Think I’ll have some time this afternoon.

…

On 20 Sep 2018, 11:41 +0200, Hugo Saracino ***@***.***>, wrote: Alright, I added the Travis CI integration as well as (some) tests. @timjs could you please rebase your branch on top of master before making changes and verifying that tests still pass ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[timjs]

Fixed the type of spacy. What do you think about using List.Nonempty for some?

[Punie]

That's a valid point !

I considered using List.Nonempty at first (when this library was only a part of a little project of mine during the 0.19 beta). When a new binary got released, List.Nonempty wasn't updated right away and so I rolled my own (which is basically the same thing).

My current position on this matter is that I don't want to tie this library to any superfluous dependency nor users to any single implementation. It should be easy for them to convert the simple (a, List a) form to any implementation of a nonempty list they fancy.

It's debatable but for the time being, I prefer sticking with the generic (a, List a) implementation.

[Punie]

Also, I'm starting to wonder if it wouldn't be more clear to implement many and by in terms of sequence for simplicity's sake.

I don't know if the following would be a performance hit (I should probably try benchmarking it) :

many : Parser a -> Parser (List a)
many item =
    sequence
        { start = ""
        , separator = ""
        , end = ""
        , spaces = spaces
        , item = item
        , trailing = Forbidden
        }

[timjs]

I'm not sure if sequence can handle the empty start and end separators correctly... Did you test it?

[Punie]

Yeah, I did it just now. Replacing the original many implementation with the one I described with sequence doesn't break any of the tests I had.

Although those might not be that exhaustive yet.

[Punie]

For reference, here is the implementation of sequence: https://github.com/elm/parser/blob/1.1.0/src/Parser/Advanced.elm#L1213
It's defined in Parser.Advanced because the Parser implementation is itself define in terms of the advanced one.

[Punie]

Anyway, sorry for the digression, that's a discussion for further down the road.

@timjs for now, and to summarize, could you please revert the change to some using List.Nonempty (I would be open to discuss it further but for now I'd rather stick with (a, List a)) and then rebase your branch on top of master so I can perform the merge ?

[timjs]

I also renamed Extras to Extra but reverted it. Don't know if you want to do that now or not.

[timjs]

Some tests obviously fail, because they depend on many parsing spaces. They should be replaced by by spaces. Do you plan to do that yourself or do you want me to take a look?