feat(combinator): add separated

[vwkd]

This attempts to add a separated parser that accepts a range similar to repeat.

Towards #98.

I tried to copy as much logic as possible from existing code. I'm unfamiliar with the code and may have missed some things, e.g. checkpoint resets, infinite loop checks, or correct error types. Please double check carefully and feel free to edit if it can be improved.

[coveralls]

Pull Request Test Coverage Report for Build 6750273694

• 71 of 138 (51.45%) changed or added relevant lines in 1 file are covered.
• 4 unchanged lines in 2 files lost coverage.
• Overall coverage decreased (-0.3%) to 44.391%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/combinator/multi.rs	71	138	51.45%

Files with Coverage Reduction	New Missed Lines	%
src/ascii/mod.rs	1	52.55%
src/combinator/multi.rs	3	61.08%

Totals
Change from base Build 6722478717:	-0.3%
Covered Lines:	1211
Relevant Lines:	2728

---

💛 - Coveralls

[epage]

Thanks for moving this forward!

[github-advanced-security]

clippy found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

[vwkd]

Hope this is now better and easier to review. Just to stress again, I'm not sure how correct this code is. Please review carefully.

Currently, the infinite loop checks for the element parser (not the separator parser) in separated0_ make one test fail.

[epage]

Hope this is now better and easier to review. Just to stress again, I'm not sure how correct this code is. Please review carefully.

Currently, the infinite loop checks for the element parser (not the separator parser) in separated0_ make one test fail.

This PR should not be changing behavior and duplicating the separated0 and separated1 bodies, its hard to tell what behavior changed for us to talk about it.

[vwkd]

I must have misunderstood the request to keep separated0 and deprecating it.

As requested, I've changed it back such that seperated0 becomes separated0_ (and similar for 1). I've added tests, and moved the variable renames into a separate commit, such that the diff is more readable¹. I've again removed the extra infinite loop checks for the element parsers (not the existing one for the separator parsers) - which was not in the existing implementation - such that all tests pass.

Please note, as requested I kept the implementation and the updating call sites in separate commits, but since the signature of the functions change, they can't be used at the existing call sites, which means there's no way to deprecate them without duplicating them. So currently, the implementation commit (feat(combinator): add separated) won't compile. The alternative would be to squash them together like they were before.

Slightly off topic: Maybe it would make sense to adopt a squash/rebase flow instead of a merge flow, such that individual commits of the PR can be purely for ease of readability and review, since they don't need to compile as the whole PR gets squashed into a single commit on the main branch. A linear history and readable commit log is a nice bonus.

Footnotes

1. If GitHub's diff doesn't line up the functions correctly, it might be worth trying VSCode (. keyboard shortcut to open the PR in github.dev). ↩

[epage]

I must have misunderstood the request to keep separated0 and deprecating it.

As requested, I've changed it back such that seperated0 becomes separated0_ (and similar for 1). I've added tests, and moved the variable renames into a separate commit, such that the diff is more readable1. I've again removed the extra infinite loop checks for the element parsers (not the existing one for the separator parsers) - which was not in the existing implementation - such that all tests pass.

This change should not be a breaking change. You can both split out separated0_ for reuse and keep separator0. See #241.

Slightly off topic: Maybe it would make sense to adopt a squash/rebase flow instead of a merge flow, such that individual commits of the PR can be purely for ease of readability and review, since they don't need to compile as the whole PR gets

Squash/rebase workflows lose valuable history and intentionally sending someone broken commits to review can be confusing.

[vwkd]

I think I understand the request now. I've kept deprecated separated0 that reuse separated0_ internally (similar for 1) and a separated commit to remove them. This should keep each commit buildable.

[epage]

Everything should be good to go after these last couple of comments are resolved! Thanks for your work on this!

[vwkd]

Addressed the remaining comments.

To lessen the breaking change before, I had considered delaying this to a follow up proposal. But since this is now a non-breaking additive change, now would be an opportunity to choose the right name while avoiding a breaking change in the future. I think naming it separate (verb, instead of adjective separated) would fit better and also align with the existing repeat.

[epage]

If you'd like to discuss name changes, I'd recommend looking into whats been done before on #95 and posting your idea there.

[epage]

Thanks!