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

IOApp.ResourceApp #1958

Merged
merged 9 commits into from
May 30, 2021
Merged

IOApp.ResourceApp #1958

merged 9 commits into from
May 30, 2021

Conversation

RaasAhsan
Copy link

@RaasAhsan RaasAhsan commented May 11, 2021

Two convenience traits that fully realize the notion that applications are resources. The two variants mirror IOApp and IOApp.Simple -- IOApp.ResourceApp and IOApp.SimpleResourceApp respectively. I could use some help with naming; IOApp.Resource is somewhat troublesome because it overloads kernel.Resource in scope. We could define it with the fully qualified package names though.

I also took the opportunity to split out an IOAppPlatform since the shared code is growing a bit

@RaasAhsan
Copy link
Author

Might have to undo the Platform, seems like it's made mima unhappy and I've never seen those issues before :)

@RaasAhsan
Copy link
Author

Another path we can take here is to create a ResourceIOApp or ResourceApp and add a Simple to the companion object

@rossabaker
Copy link
Member

Assuming IO is still baked in, I would not call it just ResourceApp. ResourceIOApp is fine.


trait ResourceApp extends IOApp {
def runResource(args: List[String]): Resource[IO, ExitCode]
final def run(args: List[String]): IO[ExitCode] = runResource(args).use(IO.pure(_))
Copy link
Member

Choose a reason for hiding this comment

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

It feels weird doing use(IO.pure(_)) when it's usually the antipattern of using Resource.

What's the usecase for this? Most of what I've seen is doing a Resource[IO, Anything] and .useForever, as is usually the case for HTTP servers.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I almost always do useForever with Resource-based IOApps. I think that would be the more useful default

@djspiewak
Copy link
Member

IMO let's just do cats.effect.ResourceApp and cats.effect.ResourceApp.Simple, with both basically wrapping useForever. For anyone who wants to do .use(IO.pure(_)), then can just use IOApp directly.

@RaasAhsan
Copy link
Author

I've actually written several applications recently that didn't call useForever, which was the main motivation for this. useForever being the default behavior seems rather opaque to me, so I wouldn't be surprised if we have confused users later who are wondering why their application isn't terminating or releasing it's resources after it performs its function. There could be similar confusion for the alternative though

Perhaps we can expose that as another pair of traits: ResourceApp.Forever and ResourceApp.ForeverSimple ? I foresee some ambiguity in overloads but I think is a reasonable resolution for this

final def run(args: List[String]): Resource[IO, ExitCode] = run.as(ExitCode.Success)
}

trait Forever { self =>
Copy link
Author

Choose a reason for hiding this comment

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

I don't really think it makes sense to have an ExitCode here... does it? also, you'll notice it doesn't extend ResourceApp for that reason, but also because we can't override main since it's declared final for now

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I'm good with Unit / Any


ioApp.main(args)
}
}
Copy link
Author

Choose a reason for hiding this comment

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

Does a ForeverSimple or Forever.Simple seem useful? It just wouldn't take any args but not sure how appealing that is if Forever doesn't need to return an ExitCode to begin with

Copy link
Member

Choose a reason for hiding this comment

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

I guess, if even just for consistency with the other ones. I would certainly use it, since we usually take arguments from env vars.

Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

I like this as-is

@djspiewak djspiewak merged commit 161274f into typelevel:series/3.x May 30, 2021
@djspiewak
Copy link
Member

Nice work! I like this addition quite a lot

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.

5 participants