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

KTOR-6412: add AcceptEncoding constants #4217

Merged
merged 5 commits into from
Sep 12, 2024

Conversation

anton-erofeev
Copy link
Contributor

Subsystem
http

Motivation
https://youtrack.jetbrains.com/issue/KTOR-6412

Solution
Added Accept Encoding constants

@e5l
Copy link
Member

e5l commented Aug 21, 2024

@marychatte, could you please check?

Copy link
Member

@marychatte marychatte left a comment

Choose a reason for hiding this comment

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

Thanks for PR!
It's a good idea to add constants! Maybe we can use HeaderValueWithParameters as it's done already with ContentType. Because it is also can be a parameter ;q= (qvalues weighting). And please add Kdoc and some tests

@anton-erofeev
Copy link
Contributor Author

Thanks for PR! It's a good idea to add constants! Maybe we can use HeaderValueWithParameters as it's done already with ContentType. Because it is also can be a parameter ;q= (qvalues weighting). And please add Kdoc and some tests

Hi @marychatte!
I updated the PR with review points fixes, could you please take a look?

Copy link
Member

@marychatte marychatte left a comment

Choose a reason for hiding this comment

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

@anton-erofeev Good work on the PR! Thank you a lot
I added a few comments, but in general I think everything is good!

/**
* Represents the `Accept-Encoding` HTTP header, which specifies the content encoding the client is willing to accept.
*
* @property acceptEncoding The encoding type as a string, such as "gzip", "compress", or "br".
Copy link
Member

Choose a reason for hiding this comment

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

small: maybe change such as "gzip", "compress", or "br" to such as "gzip", "compress", "br", etc.

* Companion object containing predefined commonly used `Accept-Encoding` values.
*/
public companion object {
public val gzip: AcceptEncoding = AcceptEncoding("gzip")
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about having names capitalized? We have for example, ContentType.Application.Any for consistency we can have AcceptEncoding.Gzip

return this
}

return AcceptEncoding(acceptEncoding, listOf(HeaderValueParam("q", qValue.toString())))
Copy link
Member

Choose a reason for hiding this comment

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

Can we also add the next constructor:

public constructor(acceptEncoding: String, qValue: Double) : this(
        acceptEncoding,
        listOf(HeaderValueParam("q", qValue.toString()))
    )

So we can have for example AcceptEncoding("gzip", 0.2) and use it also here return AcceptEncoding(acceptEncoding, qValue)

}

@Test
fun testAcceptEncodingEquals() {
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about adding the function match? To have something like this:
AcceptEncoding.Gzip.match(AcceptEncoding.All)

Copy link
Member

@marychatte marychatte left a comment

Choose a reason for hiding this comment

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

Good work, thank you!
I will merge the PR when checks pass

@marychatte marychatte merged commit a5ef2cf into ktorio:main Sep 12, 2024
10 of 14 checks passed
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.

3 participants