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

Fixes #3313 Allow passing a ClassLoader to ResourceService and Webjar… #3373

Merged
merged 2 commits into from Jun 22, 2020
Merged

Fixes #3313 Allow passing a ClassLoader to ResourceService and Webjar… #3373

merged 2 commits into from Jun 22, 2020

Conversation

ashwinbhaskar
Copy link
Collaborator

@ashwinbhaskar ashwinbhaskar commented May 1, 2020

…Service

Copy link
Member

@rossabaker rossabaker left a comment

Nicely done. I like most of it. A couple thoughts below.

* @param preferGzipped whether to serve pre-gzipped files (with extension ".gz") if they exist
*/
final case class Config[F[_]](
class ResourceServiceBuilder[F[_]] private (
Copy link
Member

@rossabaker rossabaker May 2, 2020

Choose a reason for hiding this comment

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

I think we could move these to the top level. Then we could leave the old interface as deprecated to assist people in migration. What do you think?

Copy link
Collaborator Author

@ashwinbhaskar ashwinbhaskar May 2, 2020

Choose a reason for hiding this comment

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

@rossabaker If this is going to go in 1.0, should we not just move away from the older interface right away? It's a breaking change anyway. Why keep the older interface?

Copy link
Member

@rossabaker rossabaker May 2, 2020

Choose a reason for hiding this comment

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

I like to keep them with @deprecated annotations, so it tells people how to migrate to the new interface. People are a lot more likely to read their compiler warnings than they are the release notes.

Copy link
Collaborator Author

@ashwinbhaskar ashwinbhaskar May 3, 2020

Choose a reason for hiding this comment

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

@rossabaker got it

Copy link
Collaborator Author

@ashwinbhaskar ashwinbhaskar May 3, 2020

Choose a reason for hiding this comment

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

@rossabaker I have made the changes and pushed bulder to top level.

* @param bufferSize size hint of internal buffers to use when serving resources
* @param cacheStrategy strategy to use for caching purposes. Default to no caching.
* @param preferGzipped whether to serve pre-gzipped files (with extension ".gz") if they exist
*/
Copy link
Member

@rossabaker rossabaker May 2, 2020

Choose a reason for hiding this comment

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

I wish we didn't lose the scaladoc. If we make the constructor parameters vals, can we still attach it?

@ashwinbhaskar ashwinbhaskar requested a review from rossabaker May 4, 2020
@ashwinbhaskar
Copy link
Collaborator Author

ashwinbhaskar commented May 6, 2020

@rossabaker did you get a chance to review the PR?

Copy link
Member

@rossabaker rossabaker left a comment

This looks good to me, once we clean up the deprecation version.

@@ -32,6 +146,7 @@ object ResourceService {
preferGzipped: Boolean = false)

/** Make a new [[org.http4s.HttpRoutes]] that serves static files. */
@deprecated("use ResourceServiceBuilder", "1.0.0")
Copy link
Member

@rossabaker rossabaker May 8, 2020

Choose a reason for hiding this comment

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

This will be released in a milestone. There are a few more like it:

Suggested change
@deprecated("use ResourceServiceBuilder", "1.0.0")
@deprecated("use ResourceServiceBuilder", "1.0.0-M1")

Copy link
Collaborator Author

@ashwinbhaskar ashwinbhaskar May 8, 2020

Choose a reason for hiding this comment

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

@rossabaker made the changes

@rossabaker rossabaker added this to the 1.0.0-M1 milestone May 8, 2020
@rossabaker
Copy link
Member

rossabaker commented May 8, 2020

Sorry, one more thing. Code looks ready, but I hadn't noticed the docs don't compile. ++2.12.11 docs/tutOnly static.md should reveal what needs to be done.

@ashwinbhaskar
Copy link
Collaborator Author

ashwinbhaskar commented May 10, 2020

@rossabaker I made the changes to static.md and pushed it. But CI/Scala 2.13.1 is failing because of SSL Handshake exception

Copy link
Member

@rossabaker rossabaker left a comment

That's one of those intermittent errors. I think this is ready.

@rossabaker
Copy link
Member

rossabaker commented Jun 20, 2020

This had some fallout from the Uri.Path while we were awaiting the second review. I tried to take care of the conflicts, which were non-trivial, so please check my work.

@ashwinbhaskar
Copy link
Collaborator Author

ashwinbhaskar commented Jun 22, 2020

@rossabaker I took a look at changes pertaining to Uri.Path. LGTM.

@rossabaker rossabaker merged commit 875758d into http4s:master Jun 22, 2020
9 of 10 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.

None yet

2 participants