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

Add experimental inline instrumented defn macro #702

Closed
wants to merge 2 commits into from

Conversation

dvingo
Copy link
Contributor

@dvingo dvingo commented May 5, 2022

This came out of the discussion over here:
#695

When working in cljs having the code output by macros in the same namespace you're working in will enable repl-driven development without needing to think about refreshing the namespace where instrument! was called.

This PR adds a macro, inspired by Guardrails/Ghostwheel >defn macro, which will output an instrumented function.
A parallel noop ns can be added to allow controlling this via shadow-cljs config (see https://github.com/fulcrologic/guardrails#dead-code-elimination)

I needed to add the {:registry (m/default-schemas)} lines because the schemas used for the parsing code will not always be available if the user is using a custom registry.

@mauricioszabo
Copy link
Contributor

So, it works but there are some issues with metadata:

image

:arglists is empty, and I tried to add a docstring but it also gets removed from metadata...

@dvingo
Copy link
Contributor Author

dvingo commented May 6, 2022

thanks for trying it out (and checking for the meta)! - I will refactor that to support it. I think the >fn should be straightforward too, I'm interested to see that in action

@mauricioszabo
Copy link
Contributor

Sure, please let me know if I can help in any way.

I tried some combinations of with-meta and others, and found a sad thing: seems that it's not possible to override the :arglists element using metadata. I looked at Prismatic Schema's implementation, and indeed it uses defn under the hood, so maybe it's the only way to make it work...

Comment on lines 75 to 79
schema (as-> (map ->schema parglists) $ (if single (first $) (into [:function] $)))]
`(def ~name (m/-instrument {:schema ~schema :report ~report}
(c/fn ~name
~@(map (fn [{:keys [arglist prepost body]}] `(~arglist ~prepost ~@body)) parglists)
~@(when-not single (some->> arities val :meta vector))))))))
Copy link
Contributor

@mauricioszabo mauricioszabo May 6, 2022

Choose a reason for hiding this comment

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

I found a way to keep the meta with minimum effort, if you want to use it:

Suggested change
schema (as-> (map ->schema parglists) $ (if single (first $) (into [:function] $)))]
`(def ~name (m/-instrument {:schema ~schema :report ~report}
(c/fn ~name
~@(map (fn [{:keys [arglist prepost body]}] `(~arglist ~prepost ~@body)) parglists)
~@(when-not single (some->> arities val :meta vector))))))))
schema (as-> (map ->schema parglists) $ (if single (first $) (into [:function] $)))
meta-to-defn (cond-> meta doc (assoc :doc doc))]
`(do
(c/defn ~name ~meta-to-defn ~(map :arglist parglists))
(set! ~name
(m/-instrument {:schema ~schema :report ~report}
(c/fn ~name
~@(map (fn [{:keys [arglist prepost body]}] `(~arglist ~prepost ~@body)) parglists)
~@(when-not single (some->> arities val :meta vector)))))))))

But this code does not work with Clojure (because you can't set! root bindings). I'm supposing ->defn is considered an implementation detail, so we can send an additional parameter (like is-clojurescript?) to handle the Clojure case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for looking into this! based on that I went with a strategy to produce an actual defn that just delegates to the instrumented version.

opqdonut added a commit to opqdonut/malli that referenced this pull request Jan 30, 2023
cribbed from metosin#702

Co-authored-by: Daniel Vingo dan@danvingo.com
opqdonut added a commit to opqdonut/malli that referenced this pull request Jan 30, 2023
cribbed from metosin#702

Co-authored-by: Daniel Vingo <dan@danvingo.com>
@dvingo
Copy link
Contributor Author

dvingo commented Feb 2, 2023

This was implemented in #825

@dvingo dvingo closed this Feb 2, 2023
@dvingo dvingo deleted the experimental-cljs-defn branch February 2, 2023 21:57
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.

2 participants