Some minor nits post Parser0 change

[johnynek]

This is a minor change after #116

1. as discussed, with make orElse name the same. I was reluctant since I was nervous that scala wouldn't find the right method, but I guess it works fine.
2. a few name changes (Map1 becomes Map).
3. a couple minor optimizations (repAs in rep(min: Int), optimize .as(foo).map(fn) to be .as(foo(fn)) to avoid evaluating fn on each value.
4. Make Impl private again, lift State out and make just that private[parse]. I prefer to use the stronger encapsulation (private) if we can.

What do you think @martijnhoekstra @regadas

[johnynek]

so, it looks like 2.12 has a buggy (IMO) warning that gets triggered by this change. Then fatal warnings kills the build.

[codecov-io]

Codecov Report

Merging #123 (7a569d8) into main (1fae2eb) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #123      +/-   ##
==========================================
- Coverage   94.34%   94.28%   -0.06%     
==========================================
  Files           6        6              
  Lines         760      770      +10     
  Branches       70       67       -3     
==========================================
+ Hits          717      726       +9     
- Misses         43       44       +1     

Impacted Files	Coverage Δ
core/shared/src/main/scala/cats/parse/Parser.scala	94.68% <100.00%> (-0.23%)	⬇️
...shared/src/main/scala/cats/parse/Accumulator.scala	82.85% <0.00%> (+2.85%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fae2eb...7a569d8. Read the comment docs.

[johnynek]

This reminds me: I didn't make def |(that: Parser[A]): Parser[A] as an alias for orElse originally because there was orElse and orElse1. Now that isn't the case any more, I think we can add that operator. I think that will be pretty nice since it is a fairly obvious and convenient operation for parsers.

Of course that does mean <+> which is the MonoidK syntax, is duplicative. I do think | is more obvious.

[martijnhoekstra]

Nice, LGTM. I don't know what the warning was exactly, but maybe that can be suppressed in a follow up when 2.12.13 is out with warning suppression (any day now).

[johnynek]

Thanks for the review @martijnhoekstra --

By the way, do you think you will have time to send a PR to scala steward to set your scala fix rule as the rule to apply to upgrade to 0.3.0?

[regadas]

Looks great!

the orElse looks nice. Adding an operator would look nice as well. I like | but I wonder about the user's familiarity with <+>. Also, the idea here would be to add or replace?

[johnynek]

we can't replace <+> since that is cats syntax for MonoidK combine. It is a bit hard to think of a symbol that works for everything, and that's the one they chose. | works well for some types, but would be bad for List, for example.

So I guess I'll make a second PR that adds | here as an alternative to orElse.

[martijnhoekstra]

Thanks for the review @martijnhoekstra --

By the way, do you think you will have time to send a PR to scala steward to set your scala fix rule as the rule to apply to upgrade to 0.3.0?

Yeah, sure, I'll do that for tomorrow. I'll just point it to the head of the branch, so I can still make the change for orElse (or any other changes that may be part of 0.3.0)