v.0.2.0 continued #52

sergei-shabanau · 2019-10-01T20:13:21Z

Closes #49

Added functionality and example of state being passed through the parser to track parser position and accumulate errors.

This brings the current version of library at par with version 0.1.

codecov-io · 2019-10-01T20:19:29Z

Codecov Report

Merging #52 into master will increase coverage by 0.3%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master      #52     +/-   ##
=========================================
+ Coverage   97.59%   97.89%   +0.3%     
=========================================
  Files           7        7             
  Lines         291      333     +42     
  Branches        7        9      +2     
=========================================
+ Hits          284      326     +42     
  Misses          7        7

Impacted Files	Coverage Δ
...ain/scala/org/spartanz/parserz/ParsersModule.scala	`100% <100%> (ø)`	⬆️

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 c2d42d2...06a6487. Read the comment docs.

jdegoes · 2019-10-06T23:05:44Z

Will take a look tomorrow, sorry for the delay on this!

sergei-shabanau · 2019-10-07T13:46:57Z

Thanks @jdegoes !
It's based on the code you showed me in Boulder, with all the functionality from v.0.1 implemented with that new approach.

soujiro32167

FWIW =]

ryanonsrc

Hey there. So, I am now returning from being "off the grid" so I will now take a look.

ryanonsrc · 2019-10-08T08:25:32Z

src/main/scala/org/spartanz/parserz/ParsersModule.scala

+    final def mapOptional[SI1 <: SI, SO1 >: SO, E1 >: E, B](
+      es: SI1 => (SO1, E1)
+    )(to: A => Option[B], from: B => Option[A]): Grammar[SI1, SO1, E1, B] =
+      MapES[SI1, SO1, E1, A, B](self, es, to, from)


What is the difference between MapS and MapES ?

ryanonsrc · 2019-10-08T08:34:17Z

src/main/scala/org/spartanz/parserz/ParsersModule.scala

@@ -195,6 +268,27 @@ trait ParsersModule {
          from(a._2).fold(e => s -> Left(e), b => printer(value)(s, (a._1, b)))
        }

+      case Grammar.MapS(value, _, from) =>
+        (s: S, a: (Input, A)) => {
+          val (s1, res1) = from(s, a._2)


I tend to favor using x._n notation sparingly. Especially here, I think it might help with clarity (at the cost of brevity) to split a into Input and A values (perhaps call them ai and aa respectively).

... In fact, I notice you doing this above. I'd argue in favor of doing the same here. for at the very least: consistency's sake

@ryanonsrc I did some renaming as you suggested, thanks!

ryanonsrc

Added some comments. I can't say that I fully grok the new impl. yet but that's ok, I do like to see this moving towards a more user-friendly implementation. Approving from my end but curious to hear others, and @jdegoes to chime in.

ryanonsrc · 2019-10-08T08:35:25Z

src/main/scala/org/spartanz/parserz/ParsersModule.scala

@@ -195,6 +268,27 @@ trait ParsersModule {
          from(a._2).fold(e => s -> Left(e), b => printer(value)(s, (a._1, b)))
        }

+      case Grammar.MapS(value, _, from) =>
+        (s: S, a: (Input, A)) => {
+          val (s1, res1) = from(s, a._2)


... In fact, I notice you doing this above. I'd argue in favor of doing the same here. for at the very least: consistency's sake

jdegoes · 2019-10-08T11:03:24Z

src/main/scala/org/spartanz/parserz/ParsersModule.scala

-      mapPartial[E1, A](e)({ case a if f(a) => a }, { case a if f(a) => a })
+      Map[SI, SO, E1, A, A](self, asEither(e)(Some(_).filter(f)), asEither(e)(Some(_).filter(f)))
+
+    final def mapS[SI1 <: SI, SO1 >: SO, B](to: (SI1, A) => (SO1, B), from: (SI1, B) => (SO1, A)): Grammar[SI1, SO1, E, B] =


I'd tend toward the more verbose, and call this mapStatefully.

jdegoes · 2019-10-08T11:03:37Z

src/main/scala/org/spartanz/parserz/ParsersModule.scala

+        { case (si, b) => val (so, a) = from(si, b); (so, Right(a)) }
+      )
+
+    final def mapPartialS[SI1 <: SI, SO1 >: SO, E1 >: E, B](


mapStatefullyPartial

jdegoes · 2019-10-08T11:03:52Z

src/main/scala/org/spartanz/parserz/ParsersModule.scala

+    )(to: (SI1, A) =?> (SO1, B), from: (SI1, B) =?> (SO1, A)): Grammar[SI1, SO1, E1, B] =
+      MapS[SI1, SO1, E1, A, B](self, asEither(es)(to.lift), asEither(es)(from.lift))
+
+    final def mapOptional[SI1 <: SI, SO1 >: SO, E1 >: E, B](


mapOption

jdegoes · 2019-10-08T11:05:18Z

src/main/scala/org/spartanz/parserz/ParsersModule.scala

@@ -52,6 +75,8 @@ trait ParsersModule {
    private[parserz] case class Delay[SI, SO, E, A](delayed: () => Grammar[SI, SO, E, A]) extends Grammar[SI, SO, E, A]
    private[parserz] case class Tag[SI, SO, E, A](value: Grammar[SI, SO, E, A], tag: String) extends Grammar[SI, SO, E, A]
    private[parserz] case class Map[SI, SO, E, A, B](value: Grammar[SI, SO, E, A], to: A => E \/ B, from: B => E \/ A) extends Grammar[SI, SO, E, B]
+    private[parserz] case class MapS[SI, SO, E, A, B](value: Grammar[SI, SO, E, A], to: (SI, A) => (SO, E \/ B), from: (SI, B) => (SO, E \/ A)) extends Grammar[SI, SO, E, B]


This one can be implemented in terms of MapES, right? Also, Map can be implemented in terms of MapES.

Unless there is an advantage on the implementation side to separating these cases (and sometimes there is!), it may be best to support a relatively small set of operations, to make maintenance easier.

@jdegoes
Hi John! Thanks a lot for the review!

The reason I have several Maps is the way state type parameters SI and SO are defined.

I cannot implement Map in terms of MapS because Map captures functions like A => B or A => Option[B], but MapS is about functions (SI, A) => (SO, B) etc.
I cannot build such function from just A => B because I have no knowledge of how to go from SI to SO since those types are completely unrelated.

Thus Map and MapS have to be handled at the implementation level, where SI and SO are restricted further.

MapES is there for convenient use case: when mapping functions are not dealing with state, but error manipulations require state changes. That is why MapES captures SI => (SO, E).
I faced the same problem of not knowing how to go from SI to SO when trying to express it in terms of MapS (unless asking for extra function from user, and that wasn't friendly), and eventually ended up with 3 different mapping constructs.

Makes sense. 👍

jdegoes · 2019-10-08T11:06:34Z

src/main/scala/org/spartanz/parserz/ParsersModule.scala

@@ -67,12 +92,27 @@ trait ParsersModule {
    final def fail[E, A](e: E): Grammar[Any, Nothing, E, A] =
      unit.mapPartial(e)(PartialFunction.empty, PartialFunction.empty)

+    final def fail[SI, SO, E, A](es: SI => (SO, E)): Grammar[SI, SO, E, A] =
+      unit.mapPartial(es)(PartialFunction.empty, PartialFunction.empty)


Maybe failStatefully, so you can have straight-up fail that doesn't change state.

jdegoes · 2019-10-08T11:06:45Z

src/main/scala/org/spartanz/parserz/ParsersModule.scala

    final def consume[SI, SO, E, A](
      to: (SI, Input) => (SO, E \/ (Input, A)),
      from: (SI, (Input, A)) => (SO, E \/ Input)
    ): Grammar[SI, SO, E, A] =
      Consume(to, from)

+    final def consumeOptional[SI, SO, E, A](e: E)(


consumeOption?

jdegoes · 2019-10-08T11:07:01Z

src/main/scala/org/spartanz/parserz/ParsersModule.scala

+          val (so, r) = from(si, x); (so, r.map(Right(_)).getOrElse(Left(e)))
+        }
+      )
+
    final def consume0[E, A](to: Input => E \/ (Input, A), from: ((Input, A)) => E \/ Input): Grammar[Any, Nothing, E, A] =


consumeOrFail?

jdegoes · 2019-10-08T11:10:24Z

src/main/scala/org/spartanz/parserz/ParsersModule.scala

@@ -21,7 +21,30 @@ trait ParsersModule {
      Map[SI, SO, E1, A, B](self, asEither(e)(to.lift), asEither(e)(from.lift))


Everywhere in this file, I'd make sure you are aggressively lazy, so we can have recursive grammars. It will require a trick (identity map) to derive the graph from that, but it can be done.

I have implemented recursive grammars with the explicit Delay and corresponding combinator, please see

parserz/src/main/scala/org/spartanz/parserz/ParsersModule.scala

Lines 84 to 85 in c2d42d2

final def delay[SI, SO, E, A](g: => Grammar[SI, SO, E, A]): Grammar[SI, SO, E, A] =

Delay(() => g)

I'll try to allow recursion out of the box, without additional work at the call site. Thanks!

jdegoes

I'd like to do a full review sometime over the whole API and make final suggestions. But this is going to be one kick-ass parser combinator library!

sergei-shabanau · 2019-10-09T00:05:59Z

@jdegoes
I'd love to get more feedback from you, maybe we can have a review session at some point during ScalaIO?
Thanks again!

jdegoes · 2019-10-09T00:07:10Z

@sergei-shabanau Yes, let's do it, I'll be around the whole conference!

#49 - State example works 🎉🎉🎉

a02b9d6

sergei-shabanau changed the title ~~#49 - State example works 🎉🎉🎉~~ v.0.2.0 continued Oct 1, 2019

#49 - Added more tests

8510e56

sergei-shabanau requested review from jdegoes, regis-leray, ryanonsrc, scoquelin and soujiro32167 October 4, 2019 22:27

#49 - Added even more tests

fa0b65f

regis-leray approved these changes Oct 6, 2019

View reviewed changes

soujiro32167 approved these changes Oct 7, 2019

View reviewed changes

ryanonsrc reviewed Oct 8, 2019

View reviewed changes

ryanonsrc approved these changes Oct 8, 2019

View reviewed changes

jdegoes reviewed Oct 8, 2019

View reviewed changes

jdegoes approved these changes Oct 8, 2019

View reviewed changes

#49 - Renamed few things for consistency

06a6487

sergei-shabanau mentioned this pull request Oct 9, 2019

Allow recursive grammars without the explicit use of Delay #53

Open

sergei-shabanau merged commit e73afe1 into master Oct 9, 2019

sergei-shabanau deleted the 49-v.0.2.0-cont branch October 9, 2019 00:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

v.0.2.0 continued #52

v.0.2.0 continued #52

sergei-shabanau commented Oct 1, 2019 •

edited

codecov-io commented Oct 1, 2019 •

edited

jdegoes commented Oct 6, 2019

sergei-shabanau commented Oct 7, 2019

soujiro32167 left a comment

ryanonsrc left a comment

ryanonsrc Oct 8, 2019

ryanonsrc Oct 8, 2019

ryanonsrc Oct 8, 2019

sergei-shabanau Oct 9, 2019

ryanonsrc left a comment

ryanonsrc Oct 8, 2019

jdegoes Oct 8, 2019

jdegoes Oct 8, 2019

jdegoes Oct 8, 2019

jdegoes Oct 8, 2019

sergei-shabanau Oct 8, 2019

jdegoes Oct 8, 2019

jdegoes Oct 8, 2019

jdegoes Oct 8, 2019

jdegoes Oct 8, 2019

jdegoes Oct 8, 2019

sergei-shabanau Oct 9, 2019

sergei-shabanau Oct 9, 2019

jdegoes left a comment

sergei-shabanau commented Oct 9, 2019

jdegoes commented Oct 9, 2019

		@@ -21,7 +21,30 @@ trait ParsersModule {
		Map[SI, SO, E1, A, B](self, asEither(e)(to.lift), asEither(e)(from.lift))

	final def delay[SI, SO, E, A](g: => Grammar[SI, SO, E, A]): Grammar[SI, SO, E, A] =
	Delay(() => g)

v.0.2.0 continued #52

v.0.2.0 continued #52

Conversation

sergei-shabanau commented Oct 1, 2019 • edited

codecov-io commented Oct 1, 2019 • edited

Codecov Report

jdegoes commented Oct 6, 2019

sergei-shabanau commented Oct 7, 2019

soujiro32167 left a comment

Choose a reason for hiding this comment

ryanonsrc 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

ryanonsrc 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

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

jdegoes left a comment

Choose a reason for hiding this comment

sergei-shabanau commented Oct 9, 2019

jdegoes commented Oct 9, 2019

sergei-shabanau commented Oct 1, 2019 •

edited

codecov-io commented Oct 1, 2019 •

edited