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

All backends write headers as UTF-8 #2108

Merged
merged 8 commits into from
Mar 19, 2024
Merged

All backends write headers as UTF-8 #2108

merged 8 commits into from
Mar 19, 2024

Conversation

ghik
Copy link
Contributor

@ghik ghik commented Mar 15, 2024

@ghik ghik changed the base branch from master to v3 March 15, 2024 11:39
@ghik ghik requested a review from adamw March 15, 2024 11:40
Comment on lines 5 to 7
abstract class AsyncHttpClientHttpTest[F[_]] extends HttpTest[F] {
override protected def supportsNonAsciiHeaderValues = false
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to extract this class because it makes it clear that it's the AsyncHttpClient that does not support non-ascii header fields, which would not be apparent if this method was overridden in every AsyncHttpClient*HttpTest.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good idea, but probably there are more such common flags that are set in each test class individually which could be de-duplicated here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll go through them and see what else I can extract.

@ghik ghik marked this pull request as ready for review March 18, 2024 14:30
@ghik
Copy link
Contributor Author

ghik commented Mar 18, 2024

@adamw seems like sttp-model dropped support for Scala 2.11 while sttp still retains it. What should we do?

@adamw
Copy link
Member

adamw commented Mar 18, 2024

Ah it's the v3 branch ... let's drop 2.11 here as well, since we got so far ;)

@ghik
Copy link
Contributor Author

ghik commented Mar 18, 2024

Or maybe a more sensible thing to do would be to backport the sttp-model changes to the version used in sttp3, i.e. 1.5.x, while using 1.7.9 in master of sttp?

@adamw
Copy link
Member

adamw commented Mar 18, 2024

I think we'll be maintaining the v3 branch for a while, and maintaining too many forks across projects is just too much work. So if we need to drop 2.11, let's do it :)

@ghik
Copy link
Contributor Author

ghik commented Mar 18, 2024

There would be no forks involved, only a bugfix targeting a slightly earlier version (1.5.x instead of 1.7.x, which is probably what I should have done from the beginning). But if you think it's fine to drop 2.11 for v3 and use newest sttp-model then sure, I won't hesitate :)

@adamw
Copy link
Member

adamw commented Mar 18, 2024

My worry is that this won't be the last fix in sttp-model that we'll want to use in sttp3, so we'll have to maintain two sttp branches (v3 & main), plus two sttp-model branches. I don't want to go that route ;)

Copy link
Member

@adamw adamw left a comment

Choose a reason for hiding this comment

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

Looks good :)

@ghik ghik merged commit 11cbab6 into v3 Mar 19, 2024
15 checks passed
@ghik ghik deleted the headers-utf8 branch March 19, 2024 16:47
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.

Browser compatibility rendering support for Content-Disposition header
2 participants