Introducing cats-parse #3855
Introducing cats-parse #3855
Conversation
There was a problem hiding this comment.
Spamming the cats-parse contributors, who might be interested in early usage. Thanks, and unsubscribe button is on your right. 😄
/cc @johnynek @regadas @Slakah @stephenjudkins @zmccoy @non @mpilquist
| new Parser(s).HttpVersion.run()(Parser.DeliveryScheme.Either).leftMap { _ => | ||
| ParseFailure("Invalid HTTP version", s"$s was not found to be a valid HTTP version") | ||
| parser.parseAll(s).leftMap { case e => | ||
| ParseFailure("Invalid HTTP version", e.toString) |
There was a problem hiding this comment.
Is there a general pretty printer for Error? Is such a thing possible? We didn't offer much detail in the parboiled implementation, and don't have to here. Just exploring what's possible.
There was a problem hiding this comment.
There is not. The expectations are sorted but there isn't much pretty about it. I have some utilities for this in bosatsu I can pull out (mostly about pointing at the part of the string that failed). That would be good to add to cats-parse.
| new HttpVersion(major.toInt, minor.toInt) | ||
| } | ||
| } | ||
| private val parser: Parser1[HttpVersion] = { |
There was a problem hiding this comment.
Some of our parsers have existed on the companion of what they're parsing so they can access a private constructor after validation. It prevents a private [http4s] unsafeFromVersion(Int, Int).
Do we like this pattern or no? The lack of inheritance perhaps makes it more palatable than it was.
There was a problem hiding this comment.
I use this pattern in internal libs a lot, and have found it quite useful.
There was a problem hiding this comment.
As long as we have no circular dependencies between parsed types, I like it, too. No way to construct an invalid one outside this compilation unit.
rossabaker
requested review from
aeons,
ChristopherDavenport,
cquiroz,
Daenyth,
hamnis,
satorg and
SystemFw
| mimaBinaryIssueFilters ++= Seq( | ||
| ProblemFilters.exclude[MissingClassProblem]("org.http4s.HttpVersion$Parser"), | ||
| ), |
There was a problem hiding this comment.
We have four options here:
- Deprecate this, and leave dead code in Dotty that would explode.
- Deprecate and move to a scala-2 directory. We'd need an
HttpVersionCompatmixin. - Remove it and grant MiMa exceptions as necessary.
- Surrender and call this branch 0.22, which might be necessary anyway.
On Gitter, @ChristopherDavenport and I overlapped on choice 3, so I'm pursuing that here. None of these choices are happy, so inviting more feedback.
There was a problem hiding this comment.
3 or 4 seems like the way to go to me.
There was a problem hiding this comment.
I was in favor of 2 or 3, and Chris liked 3 or 4.
I think the thing that may force an 0.22 is several integrations not publishing for Dotty (e.g. Play, Twirl), so we have to split up the repo, and therefore the release cycles.
|
Going to retarget this for the dotty branch. It works on series/0.21, but we don't want to introduce an 0.x core dependency there that doesn't materially improve users' lives. This is motivated by Dotty, so let's aim for a series of small PRs to that branch and divvy the work among the interested. |
rossabaker
force-pushed
the
cats-parse
branch
from
ff9e276
to
f938aa2
Compare
| val genNonTchar = frequency( | ||
| 4 -> oneOf(Set(0x00.toChar to 0x7f.toChar: _*) -- tchars), | ||
| 1 -> oneOf(0x100.toChar to Char.MaxValue) | ||
| ) |
There was a problem hiding this comment.
Something like this turns out to be a much more interesting generator than extended charsets in a lot of HTTP parsing cases. We could make some considerable improvements throughout this file.
|
I think we've laid down some good patterns here, and I'd like to get this merged and keep chipping away at individual parsers before this grows out of control. |
HttpVersionis not a particularly interesting class to parse, but I'm going to bikeshed it while we're setting precedent.