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.

Sign up for GitHub

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

Jump to bottom

add flow/for macro #142

Merged
merged 2 commits into from
Oct 29, 2020
Merged

add flow/for macro #142

merged 2 commits into from
Oct 29, 2020

Conversation

dchelimsky
Copy link
Contributor

@dchelimsky dchelimsky commented Oct 29, 2020 •
edited
Loading

Problem: apply the same flow to different sets of inputs

Some existing solutions:

  • defn a fn that returns a flow and invoke it from a defflow
(defn a-flow [arg] (flow (str "do something with " arg) ,,,)

(defflow do-something-with-x (a-flow 'x))
(defflow do-something-with-y (a-flow 'y))
;; or
(defflow do-something-with-x-and-y
  (a-flow 'x)
  (a-flow 'y))
  • use cats.core/sequence directly
(defflow do-something-with-x-and-y
  (cats.core/sequence
    (for [z [:x :y]]
      (flow (str "do stuff with " z)
       ,,,
       ))))

Proposal

This PR adds a state-flow.api/for macro that works just like clojure.core/for, but returns a flow which wraps a sequence of flows, e.g.

(defflow do-something-with-x-and-y
  (flow/for [z [:x :y]]
    (flow (str "do stuff with " z)
     ,,,
     ))))

TODO:

  • more doc

@dchelimsky dchelimsky requested review from philomates, sovelten and a team October 29, 2020 16:15
@dchelimsky dchelimsky force-pushed the dc/for branch 4 times, most recently from deeb953 to 16ea6ef Compare October 29, 2020 16:30
@sovelten
Copy link
Collaborator

I don't think I would be a user of this macro, but I understand if people want to use it.

I do think this is general enough to be part of https://github.com/nubank/cats/blob/master/src/cats/core.cljc though. It works for any monad, so it is kind of syntactic sugar for m/sequence

@dchelimsky
Copy link
Contributor Author

I don't think I would be a user of this macro, but I understand if people want to use it.

I do think this is general enough to be part of https://github.com/nubank/cats/blob/master/src/cats/core.cljc though. It works for any monad, so it is kind of syntactic sugar for m/sequence

I'm happy to submit a separate PR for cats, but I also like the idea of having this in state-flow so it's obvious to state-flow users.

@dchelimsky
Copy link
Contributor Author

it is kind of syntactic sugar for m/sequence

I agree, but I think that (m/sequence (for [,,,] many-lines-of-flows)) is easier to scan than (m/sequence (map (fn [,,,] many-lines-of-flows) seq-of-args), so I want to encourage for for consistency.

@dchelimsky
Copy link
Contributor Author

I don't think I would be a user of this macro, but I understand if people want to use it.
I do think this is general enough to be part of https://github.com/nubank/cats/blob/master/src/cats/core.cljc though. It works for any monad, so it is kind of syntactic sugar for m/sequence

I'm happy to submit a separate PR for cats, but I also like the idea of having this in state-flow so it's obvious to state-flow users.

funcool/cats#241

sovelten
sovelten reviewed Oct 29, 2020
View reviewed changes
src/state_flow/api.clj
@@ -38,3 +40,22 @@

(import-fn state-flow.state/gets get-state)
(import-fn state-flow.state/modify swap-state)

(defmacro for
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be implemented here or in another namespace, such as in state-flow.state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not really a state fn, but it's not really core for state-flow either. We could add a new namespace like state-flow.monad, but that might be confusing/distracting. Other ideas?

What about state-flow.compose? That might encourage thinking about some other ideas for composing flows.

Copy link

Choose a reason for hiding this comment

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

I vote for state-flow.compose

Copy link
Collaborator

Choose a reason for hiding this comment

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

the state is actually a state monad so there should be the place IMO, it is in the same spirit of fmap. Not that I have a strong opinion about it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In case you want to use this helper in core.clj, core.clj depends on the state.clj, but it should not depend on state-flow.api, the hierarchy looks like this:

state
|
core
|
api

things that are more fundamental than core go in the state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The struggle I'm having is that e.g. swap-state is clearly about state, whereas for is about composing flows. I've already merged this but we can always move it somewhere else later and reference it from api as we do with all the other functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whereas for is about composing flows.

And composing flows seems more fundamental than even state to me :)

@dchelimsky dchelimsky marked this pull request as ready for review October 29, 2020 19:41
@FelipeCortez
Copy link

I don't think I would be a user of this macro, but I understand if people want to use it.
I do think this is general enough to be part of https://github.com/nubank/cats/blob/master/src/cats/core.cljc though. It works for any monad, so it is kind of syntactic sugar for m/sequence

I'm happy to submit a separate PR for cats, but I also like the idea of having this in state-flow so it's obvious to state-flow users.

not very long ago I struggled when migrating a test because there was nothing out-of-the-box like this in state-flow, and using cats like the documentation suggested felt a bit wrong, so +1. I think this is also in line with the idea that you don't have to know about the state monad to use state-flow

@dchelimsky dchelimsky merged commit 5231e31 into master Oct 29, 2020
@dchelimsky dchelimsky deleted the dc/for branch October 29, 2020 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sovelten sovelten sovelten approved these changes

@FelipeCortez FelipeCortez FelipeCortez approved these changes

@philomates philomates Awaiting requested review from philomates

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dchelimsky @sovelten @FelipeCortez