Make 1 the default: rename Foo1 things to Foo, and the associated Foo to Foo0 #116

martijnhoekstra · 2020-12-14T15:57:30Z

To illustrate #113

Moved the constructors and their helper wrappers to object Parser (in the easiest way possible, by loosening access), in the second to last commit, and taking things to their logical extreme in I removed parse and parseAll from Parser0: it makes little sense to run an end-parser on a string that may not consume any characters,

That the intuition works out is shown in that none of the "real world" tests needed changing other than catching that some Parser(1)'s could be Parser0's.

Todo: Fixup the fallout of submitting too early while by while writing up the body of the PR and still running tests.

codecov-io · 2020-12-14T16:02:54Z

Codecov Report

Merging #116 (0eae46e) into main (3e79914) will decrease coverage by 0.27%.
The diff coverage is 96.11%.

@@            Coverage Diff             @@
##             main     #116      +/-   ##
==========================================
- Coverage   94.59%   94.31%   -0.28%     
==========================================
  Files           6        6              
  Lines         758      756       -2     
  Branches       77       79       +2     
==========================================
- Hits          717      713       -4     
- Misses         41       43       +2

Impacted Files	Coverage Δ
...shared/src/main/scala/cats/parse/Accumulator.scala	`80.00% <85.71%> (-2.86%)`	⬇️
core/shared/src/main/scala/cats/parse/SemVer.scala	`91.66% <95.45%> (ø)`
core/shared/src/main/scala/cats/parse/Parser.scala	`94.88% <96.38%> (-0.18%)`	⬇️
...ore/shared/src/main/scala/cats/parse/Numbers.scala	`100.00% <100.00%> (ø)`
...ore/shared/src/main/scala/cats/parse/Rfc5234.scala	`100.00% <100.00%> (ø)`

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 3e79914...0eae46e. Read the comment docs.

johnynek

A few comments...

I'm still digesting how big the change is. It seems like almost every line touching the API will change.

I'd love to get unanimity from all reviewers this is worth it (@rossabaker @mpilquist @regadas )

johnynek · 2020-12-14T19:10:02Z

core/shared/src/main/scala/cats/parse/Parser.scala

    between(b, b)

  /** Internal (mutable) parsing method.
    *
    * This method should only be called internally by parser instances.
    */
-  protected def parseMut(state: Parser.Impl.State): A
+  private[parse] def parseMut(state: Parser.Impl.State): A


why does this need to be private[parse], this does weaken the encapsulation a bit.

Yes, I still want to try harder to narrow access back to the minimum. This was the easy way out to have a compiling PR showing the external API.

If we change this back to protected it still works, right?

It's now accessed from object Parser, which is a companion of a subclass. Can you do that? IMO spec says yes, compiler says no. scala/bug#2439 says working as intended.

core/shared/src/main/scala/cats/parse/Parser.scala

core/shared/src/test/scala/cats/parse/Rfc5234Test.scala

rossabaker · 2020-12-14T20:03:18Z

The MiMa failures in the build would go away with a baseVersion := "0.3".

What signfiicant adopters are we aware of? Bosatsu, http4s, anything else? Tweet and gitter about it and invite howls of pain?

I agree taking backward compatibility seriously, but I think we take it seriously by finding an API we're happy to live with long into the future than by living with something suboptimal to avoid a scalafix/sed for a few early adopters. Judging it from the example parsers in the repo, I see more suffixes removed than added, and it feels better me.

core/shared/src/main/scala/cats/parse/Parser.scala

johnynek

I'm pretty convinced this is the right move. One thing here is that the safer option: parsers that consume at least 1 thing are the easier to write. That is as it should be. It shouldn't feel like a tax to write a better parser. The tax should come when using the weaker parser (something that might return nothing).

johnynek · 2020-12-16T20:05:57Z

core/shared/src/main/scala/cats/parse/Parser.scala

-    * Without using `with1`, these methods will return `Parser` values
-    * since they are not known to return `Parser1` values instead.
+    * Without using `with1`, these methods will return `Parser0` values
+    * since they are not known to return `Parser` values instead.
    */
  def with1: Parser.With1[A] =


this is maybe the only 1 name left... I guess it is better than withNot0 or something, but still it feels like it sticks out. I guess we should keep it to minimize change.

along with flatMap01 (because there also is flatMap (which was flatMap10) and flatMap0 (which was flatMap), tailRecM1 which I just didn't see right away and should be changed for consistency, even though it's barely used in user code.

johnynek · 2020-12-16T20:07:19Z

core/shared/src/main/scala/cats/parse/Parser.scala

    between(b, b)

  /** Internal (mutable) parsing method.
    *
    * This method should only be called internally by parser instances.
    */
-  protected def parseMut(state: Parser.Impl.State): A
+  private[parse] def parseMut(state: Parser.Impl.State): A


If we change this back to protected it still works, right?

core/shared/src/main/scala/cats/parse/Parser.scala

johnynek · 2020-12-16T20:33:08Z

core/shared/src/main/scala/cats/parse/Parser.scala

@@ -2233,67 +2236,67 @@ object Parser extends ParserInstances {
 }

 abstract class ParserInstances {


since there are two companions now, is there any reason not to just make this object Parser0?

Also, should we alias all the def foo0 methods in here so people can do P0.string or P.string rather than P.string0 which is a bit weird.

For the first, I'll try it out.

For the second, I don't have a strong opinion either way. Having a P0.foo as an alias for P.foo0 meaning something subtly but breakingly different from P0.foo is a bit of a foot gun. I'm not sure if the added convenience is worth it.

I don't follow how they would be different. Can you explain? I am proposing they point to exactly the same code.

I meant to say the difference between P.foo and P0.foo being the foot gun, but in style I mistyped it.

johnynek · 2020-12-19T18:49:59Z

Is this ready to merge you think?

martijnhoekstra · 2020-12-19T21:51:12Z

flatMap and (soft)product haven't been renamed back to the 10 versions yet (unless you changed your mind about that) and I haven't looked into unifying rep and repAs yet. A RepParser[A] extends Parser[Seq[A]] with methods to set sep, accumulator, min and max also sounds nice, with the current methods as shortcuts creating them. But it's orthogonal to this change I think.

I want to check if the scaladoc is all still ok, or now refers to methods that are wrong or no longer exist, and the below comment on orElse is still unresolved.

regadas · 2020-12-19T21:59:31Z

core/shared/src/main/scala/cats/parse/Parser.scala

@@ -174,8 +174,8 @@ sealed abstract class Parser[+A] {
    * one to pick up after any error, resetting any state that was
    * modified by the left parser.
    */
-  def orElse[A1 >: A](that: Parser[A1]): Parser[A1] =
-    Parser.oneOf(this :: that :: Nil)
+  def orElse0[A1 >: A](that: Parser0[A1]): Parser0[A1] =


I was just thinking... could we rename this to just orElse? wdyt?

The result will be that Parser has this inherited orElse, and an overloaded orElse with Parser as an argument and returning a Parser. After making the change and search-and-replacing orElse0 with orElse, all the tests still pass.

So I think it's safe to say yes, we could.

I have no strong opinion either way on whether we should and whether we should in this particular PR.

The tension is between the convenience of just being able to say .orElse and getting the most specific result possible depending on the receiver and argument types and the obviousness of which method is called when one has the 0.

cc @johnynek

orElse -> orElse0, orElse1 -> orElse

Note the typeclass version. I think this is OK, but without looking very closely

the easiest way possible: by loosening access. I tried a bit to do it nicer, but not particularly hard

johnynek

This looks great. I'm inclined to merge now. And strong objections?

To me, the main arguments are:

Almost all parsers (basically except for whitespace) parse at least one character.
This makes doing the more constrained and safer thing easier and adds a tax to putting less constraint, which is the right bias IMO.

martijnhoekstra · 2020-12-20T18:40:17Z

I have a WIP for a scalafix too: https://github.com/martijnhoekstra/catsparsescalafix

It's not up to date for the latest changes yet, doesn't replace some instance methods yet (for some reason I haven't figured out yet), and sometimes leaves unused imports and sometimes clashes imports, but it greatly helps in migrating: https://github.com/johnynek/bosatsu/compare/master...martijnhoekstra:fixedCompile?expand=1 was just a few manual edits from the scalafix.

johnynek · 2020-12-20T19:04:57Z

Can you add an MD file on how to use it to migrate. It would be awesome if we could figure out how to tell @scala-steward to use that scala fix to upgrade people too.

fthomas · 2020-12-20T19:11:58Z

It would be awesome if we could figure out how to tell @scala-steward to use that scala fix to upgrade people too.

Documentation for that is here: https://github.com/scala-steward-org/scala-steward/blob/master/docs/scalafix-migrations.md#adding-migration-rules-to-scala-steward

johnynek · 2020-12-20T20:07:33Z

@martijnhoekstra can you merge main back in and I won't add anything else until this PR lands.

Then it looks like we can send a PR to scalasteward to point to the scalafix you have which hopefully we can get good enough that it is a very minor edit needed for any users.

Thanks a ton for this. It was a great idea and a great cleanup.

martijnhoekstra · 2020-12-20T20:24:23Z

@johnynek there is nothing to merge from main -- I rebased earlier today.

johnynek · 2020-12-20T20:29:13Z

Without any objections I'll merge in the morning.

regadas

LGTM, no strong objections on my side. I would like to revisit the orElse subject though.

johnynek · 2020-12-21T18:45:24Z

Thanks all! I think this is a great change to make the library easier to use and steer us more towards non-empty parsers.

martijnhoekstra changed the title ~~Make 1~~ Make 1 the default: rename Foo1 things to Foo, and the associated Foo to Foo0 Dec 14, 2020

johnynek mentioned this pull request Dec 14, 2020

1's everywhere are cumbersome #113

Closed

johnynek reviewed Dec 14, 2020

View reviewed changes

rossabaker mentioned this pull request Dec 15, 2020

Add either Parser #115

Merged

martijnhoekstra force-pushed the doubleDown branch from 18438e0 to 6d85cc6 Compare December 15, 2020 07:46

martijnhoekstra commented Dec 16, 2020

View reviewed changes

core/shared/src/main/scala/cats/parse/Parser.scala Show resolved Hide resolved

johnynek reviewed Dec 16, 2020

View reviewed changes

regadas reviewed Dec 19, 2020

View reviewed changes

martijnhoekstra added 18 commits December 20, 2020 15:05

baseversion to 0.3

1dd6ad9

rename Parser to Parser0

e91c272

rename Parser1 to Parser

f20c0ed

rename orElse

851d026

orElse -> orElse0, orElse1 -> orElse

rename rep(1), repAs(1), Accumulator(1)

1e1d654

rename class OneOf(1)

37c39fe

rename oneOf(1)

7aeb861

rename compute(1)

c2983d5

rename Defer(1)

a24f24b

rename defer(1)

f4e0443

Note the typeclass version. I think this is OK, but without looking very closely

rename Rep(1)

6c10ad2

rename unmap(1)

112cbbe

reneme void(1), string(1)

a3dd4f3

rename ignoreCase(1)

e6b9a74

rename ParserGen.gen(1)

77c71cb

ignoreCase test generator rename

cc7baeb

rename charIn(1) generators

7be1eac

rename backtrack(1)

0bb0fa3

martijnhoekstra added 13 commits December 20, 2020 15:15

rename flatMap(10)

aec9fc5

rename charseWhile(1)

e511947

rename until(1)

9715e5a

rename Soft(10)

720fe99

move constructors to Parser

f422516

the easiest way possible: by loosening access. I tried a bit to do it nicer, but not particularly hard

fixup doc

e3b28b1

fixup broken P0 replacements

c1e0222

rename digits(1)

3f0631d

undo oneOf rename for array overload

c276fc6

rename back StartParser and EndParser

9fb99ac

rename impl StringP1

aa3fa7e

fixup tailrecM

f0cccc2

object Parser0 == instances

705bb77

martijnhoekstra force-pushed the doubleDown branch from 7a2841a to 705bb77 Compare December 20, 2020 14:20

martijnhoekstra added 4 commits December 20, 2020 17:20

remove Parser#rep and Parser#rep0

7ea3863

rename flatMap back to flatMap10

0670b8d

rename product back to product10

2e30021

rename softProduct back to softProduct10

0eae46e

johnynek approved these changes Dec 20, 2020

View reviewed changes

regadas approved these changes Dec 20, 2020

View reviewed changes

johnynek merged commit ef9c471 into typelevel:main Dec 21, 2020

This was referenced Dec 21, 2020

Add stringIn1 parser #110

Merged

Some minor nits post Parser0 change #123

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make 1 the default: rename Foo1 things to Foo, and the associated Foo to Foo0 #116

Make 1 the default: rename Foo1 things to Foo, and the associated Foo to Foo0 #116

martijnhoekstra commented Dec 14, 2020 •

edited

codecov-io commented Dec 14, 2020 •

edited

johnynek left a comment

johnynek Dec 14, 2020

martijnhoekstra Dec 14, 2020

johnynek Dec 16, 2020

martijnhoekstra Dec 18, 2020

rossabaker commented Dec 14, 2020

johnynek left a comment

johnynek Dec 16, 2020

martijnhoekstra Dec 17, 2020

johnynek Dec 16, 2020

johnynek Dec 16, 2020

martijnhoekstra Dec 17, 2020

johnynek Dec 17, 2020

martijnhoekstra Dec 17, 2020

johnynek commented Dec 19, 2020

martijnhoekstra commented Dec 19, 2020 •

edited

regadas Dec 19, 2020

martijnhoekstra Dec 20, 2020

johnynek left a comment

martijnhoekstra commented Dec 20, 2020

johnynek commented Dec 20, 2020

fthomas commented Dec 20, 2020

johnynek commented Dec 20, 2020

martijnhoekstra commented Dec 20, 2020 •

edited

johnynek commented Dec 20, 2020

regadas left a comment

johnynek commented Dec 21, 2020

		@@ -2233,67 +2236,67 @@ object Parser extends ParserInstances {
		}

		abstract class ParserInstances {

Make 1 the default: rename Foo1 things to Foo, and the associated Foo to Foo0 #116

Make 1 the default: rename Foo1 things to Foo, and the associated Foo to Foo0 #116

Conversation

martijnhoekstra commented Dec 14, 2020 • edited

codecov-io commented Dec 14, 2020 • edited

Codecov Report

johnynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossabaker commented Dec 14, 2020

johnynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Dec 19, 2020

martijnhoekstra commented Dec 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek left a comment

Choose a reason for hiding this comment

martijnhoekstra commented Dec 20, 2020

johnynek commented Dec 20, 2020

fthomas commented Dec 20, 2020

johnynek commented Dec 20, 2020

martijnhoekstra commented Dec 20, 2020 • edited

johnynek commented Dec 20, 2020

regadas left a comment

Choose a reason for hiding this comment

johnynek commented Dec 21, 2020

martijnhoekstra commented Dec 14, 2020 •

edited

codecov-io commented Dec 14, 2020 •

edited

martijnhoekstra commented Dec 19, 2020 •

edited

martijnhoekstra commented Dec 20, 2020 •

edited