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

Parameterize EntityLimiter #1892

Merged
merged 2 commits into from May 30, 2018

Conversation

@lloydmeta
Copy link
Contributor

@lloydmeta lloydmeta commented May 30, 2018

Relates to #1890

import fs2._

This comment has been minimized.

@aeons

aeons May 30, 2018
Member

Imports should be ordered alphabetically (I know that's true here, but just stating it :)) and should not be grouped into java, scala, scala stdlib.

This comment has been minimized.

@jmcardon

jmcardon May 30, 2018
Member

Probably an automatic IJ thing? My commits do this too occasionally and it drives Ross mad 😄

This comment has been minimized.

@aeons

aeons May 30, 2018
Member

It's the default in IJ.

It can be changed in Settings > Editor > Code Style > Scala > Imports, by removing all lines from the Import Layout box, until only all other imports is left.

This comment has been minimized.

@rossabaker

rossabaker May 30, 2018
Member

Is there any way we can check in a project style setting for that?

If not, I might be ready to give up and do it the IntelliJ default way. (Which is a complicated and unappealing default, but not so bad it's worth asking every individual contributor to change it.)

This comment has been minimized.

@aeons

aeons May 30, 2018
Member

I'll have a look

import fs2._

This comment has been minimized.

@jmcardon

jmcardon May 30, 2018
Member

Probably an automatic IJ thing? My commits do this too occasionally and it drives Ross mad 😄

@@ -33,6 +33,7 @@ it.
* Add `BlockingHttp4sServlet` for use in Google App Engine and Servlet 2.5 containers. Rename `Http4sServlet` to `AsyncHttp4sServlet`. [#1830](https://github.com/http4s/http4s/pull/1830)
* Generalize `Logger` middleware to log with `String => Unit` instead of `logger.info(_)` [#1839](https://github.com/http4s/http4s/pull/1839)
* Rename `RequestLogger.apply0` and `ResponseLogger.apply0` to `RequestLogger.apply` and `ResponseLogger.apply`. [#1837](https://github.com/http4s/http4s/pull/1837)
* Parameterize `EntityLimiter` middleware to work on `Kleisli[F, Request[G], B]`. [#1892](https://github.com/http4s/http4s/pull/1892)

This comment has been minimized.

@jmcardon

jmcardon May 30, 2018
Member

Hell yeah Commonwealth English (Parameterize) long live the Queen! 🇬🇧

This comment has been minimized.

@SystemFw

SystemFw May 30, 2018
Member

Parameterise you mean :P

This comment has been minimized.

@rossabaker

rossabaker May 30, 2018
Member

Parameterize. With a "z". Not an "s" and there is no such thing as a "zed".

Copy link
Member

@rossabaker rossabaker left a comment

This looks good to me. The imports have gotten inconsistent between the Twitter style and IntelliJ style, so I propose that we argue about those somewhere else and do a reformat PR after a new consensus is reached.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented May 30, 2018

I see that trying to keep the changelog up to date as we go is going to cause contention. But I sure like not having to do go through 20 old PRs right before a release. 🤔

@aeons aeons force-pushed the lloydmeta:feature/parameterise-entitylimiter branch from d8ad15e to af44108 May 30, 2018
@aeons aeons merged commit 66a31ce into http4s:master May 30, 2018
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants