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

Repair example #1 for providing different service implementations #7926

Merged
merged 1 commit into from
Mar 23, 2023
Merged

Repair example #1 for providing different service implementations #7926

merged 1 commit into from
Mar 23, 2023

Conversation

pscheid92
Copy link
Contributor

No description provided.

@CLAassistant
Copy link

CLAassistant commented Mar 15, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@adamgfraser adamgfraser left a comment

Choose a reason for hiding this comment

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

The use of provideSome is more idiomatic here. It is sometimes necessary to specify the type of the services that are not being provided yet.

@@ -56,7 +56,7 @@ object MainApp extends ZIOAppDefault {
_ <- ZIO.debug(s"Application started with $poolSize pool size.")
} yield ()

- def run = myApp.provideLayer(AppConfig.appArgsLayer)
- def run = myApp.provideSome[ZIOAppArgs](AppConfig.appArgsLayer)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use provide and provideSome consistently here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is exactly what I intended here, or did I get you wrong?

We use myApp.provideSome[ZIOAppArgs](AppConfig.appArgsLayer) in the code block above. That's why I changed the diff here as well. So it explains to a reader that we now remove the configuration from app args and use system properties instead.

Remove: myApp.provideSome[ZIOAppArgs](AppConfig.appArgsLayer)
Insert: myApp.provideLayer(AppConfig.systemEnvLayer)

This is how it would be rendered:

Screenshot 2023-03-17 at 10 12 11

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. if we were using provideSome before we should use provide now.

Basically we have two "axes". One is whether we are using manual layer construction (provideLayer or provideSomeLayer) versus automatic layer construction (provide or provideSome). The second is whether we are providing all of the environment (provide or provideLayer) or some of it (provideSome or provideSomeLayer).

So these lines are somewhat confusing right now if that we are trying to illustrate how we use a different operator when we have a layer that doesn't have any other requirements but we are also changing back and forth between automatic and manual layer construction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I didn't notice!

So let's say we use automatic layer construction for this example (since all other examples on that page use it, too). Then we would have the following lines:

  • myApp.provideSome[ZIOAppArgs](AppConfig.appArgsLayer)
  • myApp.provide(AppConfig.systemEnvLayer)

For the configuration via arguments, we use provideSome since we don't provide ZIOAppArgs yet. For the configuration via system properties, we use provide, since we completely construct the layer.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I updated the commit.

@adamgfraser adamgfraser merged commit 510ab64 into zio:series/2.x Mar 23, 2023
@pscheid92 pscheid92 deleted the patch-1 branch March 23, 2023 15:09
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

3 participants