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

Relax Effect to Sync in staticcontent #3145

Merged
merged 2 commits into from Feb 4, 2020

Conversation

guymers
Copy link
Contributor

@guymers guymers commented Feb 4, 2020

All methods in the staticcontent package only require a Sync
instance.

All methods in the `staticcontent` package only require a `Sync`
instance.

object ResourceService {

/** [[org.http4s.server.staticcontent.ResourceService]] configuration
*
* @param basePath prefix of the path files will be served from
* @param blockingExecutionContext `ExecutionContext` to use when collecting content
* @param blocker `ExecutionContext` to use when collecting content
Copy link
Member

@aeons aeons Feb 4, 2020

Choose a reason for hiding this comment

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

Maybe update this to refer to cats.effect.Blocker instead of ExecutionContext, while you're at it.

Copy link
Contributor Author

@guymers guymers Feb 4, 2020

Choose a reason for hiding this comment

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

Replaced "ExecutionContext" with "execution context" to be consistent with the other docblock.

@@ -12,7 +12,7 @@ object WebjarService {

/** [[org.http4s.server.staticcontent.WebjarService]] configuration
*
* @param blockingExecutionContext execution context for blocking I/O
* @param blocker execution context for blocking I/O
Copy link
Member

@aeons aeons Feb 4, 2020

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

@guymers guymers Feb 4, 2020

Choose a reason for hiding this comment

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

Blocker is just an execution context so I think the description is valid

Copy link
Member

@rossabaker rossabaker Feb 4, 2020

Choose a reason for hiding this comment

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

I agree with @aeons here. A Blocker is a wrapper that exists to distinguish blocking execution contexts from CPU-bound execution contexts, and steers people toward better thread pool separation through types.

Copy link
Member

@rossabaker rossabaker Feb 4, 2020

Choose a reason for hiding this comment

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

Oops. GitHub folded the diff, so I only saw the doc, and not the type. I thought we were trying to use an ExecutionContext as a Blocker. I still agree with @aeons that we should not have been quoting it as an ExecutionContext, but I think this phrasing is fine.

Copy link
Contributor Author

@guymers guymers Feb 4, 2020

Choose a reason for hiding this comment

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

Not 100% sure I understand what you mean by refer to Blocker. As Blocker is the type of the parameter does it need to also be in the description? Something like "Blocker for blocking I/O"?

Copy link
Member

@rossabaker rossabaker Feb 4, 2020

Choose a reason for hiding this comment

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

I think as is, "execution context for blocking I/O", is a good description.

I would like us to avoid calling a blocker an ExecutionContext, with that capitalization, in backticks, which makes it look like a type name.

@aeons
Copy link
Member

@aeons aeons commented Feb 4, 2020

Looks good. I've added some comments, and you would need to run scalafmt as well.

Copy link
Member

@rossabaker rossabaker left a comment

👍 to most of the change, but I'd like to see Blocker.

import cats.effect._
import cats.implicits._
import cats.effect.Sync
import cats.syntax.functor._
Copy link
Member

@rossabaker rossabaker Feb 4, 2020

Choose a reason for hiding this comment

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

I'd still prefer to use cats.implicits._ here, but am open to being overruled.

@@ -12,7 +12,7 @@ object WebjarService {

/** [[org.http4s.server.staticcontent.WebjarService]] configuration
*
* @param blockingExecutionContext execution context for blocking I/O
* @param blocker execution context for blocking I/O
Copy link
Member

@rossabaker rossabaker Feb 4, 2020

Choose a reason for hiding this comment

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

I agree with @aeons here. A Blocker is a wrapper that exists to distinguish blocking execution contexts from CPU-bound execution contexts, and steers people toward better thread pool separation through types.

Copy link
Member

@rossabaker rossabaker left a comment

👍 on green, and I'm willing to put this out as an RC4.

We can squabble about the imports and the doc phrasing after bin compat is locked down.

@rossabaker rossabaker merged commit 9c5c677 into http4s:master Feb 4, 2020
2 checks passed
@rossabaker rossabaker mentioned this pull request Feb 4, 2020
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.

None yet

4 participants