-
Notifications
You must be signed in to change notification settings - Fork 400
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 brotli compression (#2646) #2857
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@@ -27,6 +27,7 @@ object Dependencies { | |||
"io.netty" % "netty-transport-native-kqueue" % NettyVersion, | |||
"io.netty" % "netty-transport-native-kqueue" % NettyVersion % Runtime classifier "osx-x86_64", | |||
"io.netty" % "netty-transport-native-kqueue" % NettyVersion % Runtime classifier "osx-aarch_64", | |||
"com.aayushatharva.brotli4j" % "brotli4j" % "1.16.0" % "provided", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very small nit: I believe we should be using optional
here instead of provided
. A good summary on the semantics from a comment here:
But, as I tried to say, the difference is mainly semantic, i.e. you signal the user that this dependency should come from the container, while for optional dependencies, you signal that you need to explicitly add the necessary dependencies
Unless of course there was some reason for using provided
in the first place then please ignore me! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the hint. I did not know optional
fixes #2646
/claim #2646