Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to repeat into different structures #45

Merged
merged 3 commits into from
Nov 6, 2020
Merged

Conversation

johnynek
Copy link
Collaborator

@johnynek johnynek commented Nov 5, 2020

I chatted with this idea with @non after seeing an example CSV parser he wrote and seeing where it could have come in handy.

def newAppender() = Appender.stringAppender()
}

implicit def listAccumulator[A]: Accumulator[A, List[A]] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have something like:

def fromBuilder[A, B](makeBuilder: () => Builder[A, B]): Accumulator[A, B] = ???

You could use that for both listAccumulator and vectorAccumulator and anyone else who wants to use Appender.fromBuilder will also want something like this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also what about:

implicit def monoidAccumulator[A](implicit ev: Monoid[A]): Accumulator[A, A] = ???

(And the same for Semigroup with Accumulator1.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lastly, I can imagine arguments for and against including Set. The argument for is that often users may want to end up with a set, and if their language ignores duplicates the behavior will be correct. The argument against is that information would be lost, and maybe the user should have to do that themselves. I could imagine building a MultiSet[A] (e.g. Map[A, Int]) although that seems kinds of loony from a documentation standpoint.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are good questions, but I left them undone for now:

  1. I think most use cases will be the ones I made, and I didn't want to add an indirection through one more lambda.
  2. As we talked offline, I can't see a good use case and plenty of weird cases for Monoid or Semigroup. For now, I'd rather punt till we have clearer examples. I don't know if it will be much more efficient than mapping and using combine by hand, which also has the benefit of reading more clearly

}
}

trait Accumulator[-A, +B] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like having Accumulator extend Accumulator1 would make sense, since:

def newAppender(first: A): Appender[A, B] = {
  val appender = newAppender()
  appender.append(first)
  appender
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call. I did this.

def repAs[B](implicit acc: Accumulator[A, B]): Parser[B] =
Parser.repAs(self)(acc)

def rep1As[B](implicit acc: Accumulator1[A, B]): Parser[B] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshedding but maybe repAs1 is better since the 1 is more obvious as a suffix (and that's where we put it in other cases).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think your suggestion is right.

Copy link
Contributor

@non non left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! 👍

@johnynek johnynek merged commit 2dbcad5 into main Nov 6, 2020
@johnynek johnynek deleted the oscar/accum branch November 6, 2020 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants