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

banner is being printed with a single logger call #1985

Merged
merged 1 commit into from Aug 11, 2018

Conversation

@pshirshov
Copy link
Contributor

@pshirshov pshirshov commented Aug 10, 2018

Fix for #1980

@@ -278,7 +278,11 @@ class BlazeBuilder[F[_]](
s"BlazeServer($address)"
}

banner.foreach(logger.info(_))
Option(banner)

This comment has been minimized.

@jmcardon

jmcardon Aug 10, 2018
Member

couldn't you just do this with a single foldLeft + an isEmpty?

This comment has been minimized.

@pshirshov

pshirshov Aug 10, 2018
Author Contributor

I may though don't see much benefits in that. This is bit more verbose but looks more idiomatic (for my taste)

This comment has been minimized.

@jmcardon

jmcardon Aug 11, 2018
Member

more of a code style thing, but if Ross doesn't mind I'll live with it.

This comment has been minimized.

@rossabaker

rossabaker Aug 11, 2018
Member

If we start doing this as a single log statement, I don't think it needs to continue to be a Seq. I think this works for 0.18 for bincompat, and we might simplify it on the server builders for 0.19.

Copy link
Member

@rossabaker rossabaker left a comment

👍 It's binary compatible, so we could cherry-pick this onto release-0.18.x and get it into a patch, which I expect we'll cut this weekend. We have a couple things queued up for that.

@jmcardon jmcardon merged commit 77c094a into http4s:master Aug 11, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
rossabaker added a commit to rossabaker/http4s that referenced this pull request Aug 11, 2018
rossabaker added a commit that referenced this pull request Aug 14, 2018
Backport #1985 to 0.18.x
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

3 participants