Skip to content

add weather cli example#16

Merged
schickling merged 7 commits intomainfrom
wttr-cli-example
Sep 25, 2023
Merged

add weather cli example#16
schickling merged 7 commits intomainfrom
wttr-cli-example

Conversation

@fubhy
Copy link
Member

@fubhy fubhy commented Sep 23, 2023

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Sep 23, 2023

⚠️ No Changeset found

Latest commit: 2041f2b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

@cjol cjol left a comment

Choose a reason for hiding this comment

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

I would personally have been inclined to put the wttr client in a dedicated Requirement (e.g. so it can be mocked in tests etc), but that's very optional!

// Based on the given options and arguments, you could invoke different (sub)commands here or
// pass different parameters to your command function. In this example, we only have a single
// command, so we just invoke it directly.
Effect.gen(function* ($) {
Copy link

Choose a reason for hiding this comment

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

It's possible that the docs are already outdated (I haven't kept up!) but the generator page suggests that _ is more of a convention than $. Worth being consistent?

The _ symbol is just a convention for the argument name and is not a special symbol in Effect. You are free to use any name you prefer (e.g., $, etc...). The current convention is to use _ as the argument name.
https://effect.website/docs/essentials/using-generators#understanding-effectgen

Copy link
Member Author

@fubhy fubhy Sep 24, 2023

Choose a reason for hiding this comment

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

I don't have a strong preference here. We've been using both $ and _ interchangeably in the other per-repo examples of the core ecosystem.

I think I have a slight preference for $ since I personally already use _ fo both "throw away" and non-name-worthy, short-lived, tightly scoped parameters in lambda functions (e.g. Effect.map(_ => _.foo)) or for pattern matching, etc.

@fubhy
Copy link
Member Author

fubhy commented Sep 24, 2023

I would personally have been inclined to put the wttr client in a dedicated Requirement (e.g. so it can be mocked in tests etc), but that's very optional!

We've had that before but decided that that was out of scope for the example (simplicity over full completess / correctness). We can't show all idiomatic patterns in one go without making it too complicated to parse for a newcomer.

@schickling schickling merged commit de8d486 into main Sep 25, 2023
@schickling schickling deleted the wttr-cli-example branch September 25, 2023 07:04
@@ -0,0 +1,77 @@
import { Effect, Option, Layer } from 'effect'
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like the Layer import is not used

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.

6 participants