with-redefs + wrap-with macros #123

caioaao · 2020-05-18T17:07:50Z

No description provided.

dchelimsky · 2020-05-18T17:11:04Z

src/state_flow/labs/state.clj

+     runner# (state-flow/runner)
+     :let [[ret# state#] (clojure.core/with-redefs ~bindings
+                           (runner# ~flow world#))]]
+    (state/put state#)


Suggested change

(state/put state#)

(state-flow.api/swap-state (constantly state#))

dchelimsky · 2020-05-18T17:11:26Z

src/state_flow/labs/state.clj

+     :let [[ret# state#] (clojure.core/with-redefs ~bindings
+                           (runner# ~flow world#))]]
+    (state/put state#)
+    (state/return ret#)))


Suggested change

(state/return ret#)))

(state-flow.api/return ret#)))

dchelimsky · 2020-05-18T17:14:55Z

I think if we're going to add this to state-flow proper (even in labs), I think we should add the wrap-in generalization. The you could use e.g.

(wrap-in (fn [wrapped-flow] (with-redefs [<bindings>] wrapped-flow))
         flow-to-wrap))

sovelten · 2020-05-18T17:16:06Z

src/state_flow/labs/state.clj

+
+(defmacro with-redefs
+  [bindings flow]
+  (assert *enable-with-redefs-macro*


Not sure about the usage of this variable. Just a warning in the comments is fine. The flag looks like a handicap to me.

sovelten · 2020-05-18T17:16:58Z

src/state_flow/labs/state.clj

@@ -0,0 +1,19 @@
+(ns state-flow.labs.state


This would probably go better in another namespace. Maybe labs.helpers

I thought it'd go along with the other state fns, like wrap-fn

I think it's fine here, but we should have a docstring in this namespace that indicates that it is experimental and subject to change.

dchelimsky · 2020-05-18T17:19:15Z

I think if we're going to add this to state-flow proper (even in labs), I think we should add the wrap-in generalization.

Background: #33

caioaao · 2020-05-20T23:43:24Z

I think if we're going to add this to state-flow proper (even in labs), I think we should add the wrap-in generalization.

@dchelimsky from what I saw, #33 was closed because of the potential of misuse. If we limit this implementation to specific use cases we see the need for it, the misuse might be reduced, no? We can keep wrap-with but as a private function

caioaao · 2020-05-20T23:47:04Z

We can also leave it public for now in labs and check if this is going to be an issue

dchelimsky · 2020-05-21T13:54:40Z

src/state_flow/labs/state.clj

+            [state-flow.state :as state]))
+
+(defmacro wrap-with
+  "WARNING: `wrap-with` usage is not recommended. Use only if you know what you're


This warning is fine, but we should also say what it does :-D

I think we should say that this here to support helper macros like with-redefs, and reference that as an implementation example.

dchelimsky · 2020-05-21T13:56:12Z

src/state_flow/labs/state.clj

+    (state/return ret#)))
+
+(defmacro with-redefs
+  "WARNING: `with-redefs` usage is not recommended. Use only if you know what you're


Same comment - docstring should also say what this does, and warn that with-redefs is not threadsafe.

dchelimsky · 2020-05-21T13:56:40Z

test/state_flow/labs/state_test.clj

+  (f))
+
+(deftest wrap-with-test
+  (testing "verify wrapper is called"


Suggested change

(testing "verify wrapper is called"

(testing "wrapper is called"

dchelimsky · 2020-05-21T13:56:50Z

test/state_flow/labs/state_test.clj

+                  (fn [f] (print "called it") (f))
+                  (state/modify put2))
+                 (state-flow/run {}))))))
+  (testing "verify flow runs successfully"


Suggested change

(testing "verify flow runs successfully"

(testing "flow runs successfully"

dchelimsky · 2020-05-21T13:57:09Z

test/state_flow/labs/state_test.clj

+                 (state-flow/run {}))))))
+
+(deftest with-redefs-test
+  (testing ""


Either fill in the docstring or bag testing

dchelimsky · 2020-05-21T14:17:49Z

Thanks for all the updates @caioaao. I'm waiting for some feedback from other folks within Nubank.

fernando-nubank · 2020-05-21T14:19:54Z

We can also leave it public for now in labs and check if this is going to be an issue

I have more issues with with-redefs than with wrap-fn but, as both of them are in labs I think they can be public.

dchelimsky

LGTM now, but I want at least one other approval before merging.

dchelimsky · 2020-05-21T14:23:02Z

LGTM now, but I want at least one other approval before merging.

Oh! There is one. I'm going to add a commit to update the changelog first. Then I'll merge it.

dchelimsky · 2020-05-21T14:24:01Z

LGTM now, but I want at least one other approval before merging.

Oh! There is one. I'm going to add a commit to update the changelog first. Then I'll merge it.

I can't do that on your branch @caioaao so I'll do it in a follow up PR before releasing.

caioaao added 2 commits May 18, 2020 13:02

with-redefs state

8cfba2c

enabled

8e0aab2

caioaao mentioned this pull request May 18, 2020

[Question] Wrapping monad with macros the right way #120

Closed

rename feature flag

c09553d

dchelimsky reviewed May 18, 2020

View reviewed changes

sovelten reviewed May 18, 2020

View reviewed changes

wrap-with helper + tests

8ef8dbe

caioaao changed the title ~~[PoC] with-redefs helper for state-flow with feature-flag for extra protection against misuse~~ with-redefs + wrap-with macros May 21, 2020

caioaao requested review from dchelimsky and sovelten May 21, 2020 00:12

dchelimsky reviewed May 21, 2020

View reviewed changes

caioaao added 2 commits May 21, 2020 10:03

docstrings

ee220ef

example

40bb113

fernando-nubank approved these changes May 21, 2020

View reviewed changes

dchelimsky approved these changes May 21, 2020

View reviewed changes

dchelimsky merged commit 73ce48f into nubank:master May 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

with-redefs + wrap-with macros #123

with-redefs + wrap-with macros #123

caioaao commented May 18, 2020

dchelimsky May 18, 2020

dchelimsky May 18, 2020

dchelimsky commented May 18, 2020

sovelten May 18, 2020

sovelten May 18, 2020

caioaao May 20, 2020

dchelimsky May 21, 2020

dchelimsky commented May 18, 2020

caioaao commented May 20, 2020

caioaao commented May 20, 2020

dchelimsky May 21, 2020

dchelimsky May 21, 2020

dchelimsky May 21, 2020

dchelimsky May 21, 2020

dchelimsky May 21, 2020

dchelimsky May 21, 2020

dchelimsky commented May 21, 2020

fernando-nubank commented May 21, 2020

dchelimsky left a comment

dchelimsky commented May 21, 2020

dchelimsky commented May 21, 2020

	(state/put state#)
	(state-flow.api/swap-state (constantly state#))

	(testing "verify wrapper is called"
	(testing "wrapper is called"

	(testing "verify flow runs successfully"
	(testing "flow runs successfully"

with-redefs + wrap-with macros #123

with-redefs + wrap-with macros #123

Conversation

caioaao commented May 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchelimsky commented May 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchelimsky commented May 18, 2020

caioaao commented May 20, 2020

caioaao commented May 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dchelimsky commented May 21, 2020

fernando-nubank commented May 21, 2020

dchelimsky left a comment

Choose a reason for hiding this comment

dchelimsky commented May 21, 2020

dchelimsky commented May 21, 2020