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

Port TransferCoding/Transfer-Encoding to cats-parse #4023

Merged
merged 4 commits into from Dec 18, 2020

Conversation

aeons
Copy link
Member

@aeons aeons commented Dec 17, 2020

See #3984

I don't have the complete overview of what is being done on which branch, so I hope noone has done this already.

I added a little helper, that I could envision being used in many of these header parsers, I can remove that again if we want to avoid that kind of thing.

@aeons aeons force-pushed the transfer-encoding-cats-parse branch from ecd40f7 to 256b7c6 Compare December 17, 2020 14:00
@aeons aeons force-pushed the transfer-encoding-cats-parse branch from 256b7c6 to 02df3dc Compare December 17, 2020 14:07
@novakov-alexey-zz
Copy link
Contributor

I think you took non-converted files, so I believe nobody has covered them yet. Please notify in the thread #3984 that few more parsers are covered.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

Nice. I particularly like having fromParser helper. That was overdue, and we should retrofit the others onto it. Small tweaks below.

@@ -77,6 +78,10 @@ object ParseResult {
case NonFatal(e) => Left(ParseFailure(sanitized, e.getMessage))
}

private[http4s] def fromParser[A](parser: Parser[A], errorMessage: => String)(
s: String): ParseResult[A] =
parser.parseAll(s).leftMap(e => ParseFailure(errorMessage, e.toString))
Copy link
Member

Choose a reason for hiding this comment

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

It's nice to centralize this, because we're going to need to do better than that e.toString.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, this was what I saw in the other places this pattern was used. Will investigate.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, what we get out of Parser.Error is an offset where the parsing failed, and a list of expected inputs that it didn't find. I can format that in a decently nice way, but it's not going be as good as the ErrorFormatter from parboiled.

An alternative is to do something like https://github.com/http4s/http4s/blob/main/core/src/main/scala/org/http4s/HttpVersion.scala#L58 and repeat the input instead of giving parser-specific errors.

Copy link
Member

Choose a reason for hiding this comment

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

The new LocationMap in cats-parse helps turn input into a row-column index and is designed to help with formatting. Oscar mentioned some code in bosatsu that formats errors, but I haven't gone hunting for it.

I suspect there's an opportunity to do a decent default rendering in cats-parse on that LocationMap.

@aeons aeons mentioned this pull request Dec 17, 2020
34 tasks
@hamnis hamnis merged commit 6135031 into http4s:dotty Dec 18, 2020
@aeons aeons deleted the transfer-encoding-cats-parse branch May 25, 2021 12:53
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

4 participants