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

Fix #302: babashka compatibility #718

Merged
merged 1 commit into from
Jul 1, 2022
Merged

Conversation

borkdude
Copy link
Contributor

@borkdude borkdude commented Jun 30, 2022

This PR relies on bb master which adds compatibility with malli. Fixes #302.

Notes:

  • To make malli work well with bb, the :bb reader conditionals rely less on internal details of Clojure and use core functions instead, similar to what happens in :cljs branches. As discussed with @puredanger, I didn't add LazilyPersistentVector to bb as that was too far off from what is considered a public API.
  • Upgrade borkdude/dynaload which was made compatible with bb
  • Add bb test runner to ensure no breakage happens with future changes to malli
  • The evaluation part is implemented by just using load-string in bb, since SCI itself isn't exposed yet (might happen in the future). SCI also has eval-form which lets you skip the string parsing bit, that might be more performant, but we can address that in a future PR.

@ikitommi I'm not sure what the workflow is with metosin/malli but I usually squash+merge the commits through the Github option (dropdown in merge button). If you want I can squash this PR manually as well.

If you want to try:

bash <(curl -s https://raw.githubusercontent.com/babashka/babashka/master/install) --version 0.8.157-SNAPSHOT --dir /tmp

for installing bb master in /tmp.
Then in a script:

(ns malli-test
  (:require [babashka.deps :as deps]))

(deps/add-deps '{:deps {metosin/malli
                        {:git/url "https://github.com/borkdude/malli"
                         :git/sha "1ec8582e3a39c8d170cf5f8a7482386bf66370fe"}}})

(require '[malli.core :as malli])

(prn (malli/validate [:map [:a [:int]]] {:a 1}))
(prn (malli/explain [:map [:a [:int]]] {:a "foo"}))

@borkdude
Copy link
Contributor Author

Sorry, had to download bb master instead of using the setup-clojure one.

@ikitommi
Copy link
Member

ikitommi commented Jul 1, 2022

Looks great and agree on the design/implementation decisions. Happy to merge this - not squashing by default (one can track the merge commits from master), but the fact "This PR relies on bb master" - should I wait for a new release of bb first?

Update to README would be great. How do you promote a lib to be compliant with bb? is it a third target like this:

| Data-driven Schemas for Clojure/Script and Babashka.

... or just a feature:

| * Works with Babashka

If you have time to update the README, would love that. Will do that otherwise.

and, THANKS.

Tommi

@borkdude borkdude force-pushed the babashka branch 6 times, most recently from 1bd589e to 6fb9adb Compare July 1, 2022 10:04
This commit adds compatibility with babashka. Fixes metosin#302.

Notes:

- To make malli work well with bb, the :bb reader conditionals rely less on internal details of Clojure and use core functions instead, similar to what happens in :cljs branches. As discussed with @puredanger, I didn't add LazilyPersistentVector to bb as that was too far off from what is considered a public API.
- Upgraded borkdude/dynaload which was made compatible with bb
- Add bb test runner to ensure no breakage happens with future changes to malli
- The evaluation part is implemented by just using load-string in bb, since SCI itself isn't exposed yet (might happen in the future). SCI also has eval-form which lets you skip the string parsing bit, that might be more performant, but we can address that in a future PR.
@borkdude
Copy link
Contributor Author

borkdude commented Jul 1, 2022

@ikitommi Addressed feedback: squashed commit and updated README.

You can see how the README looks here:

https://github.com/borkdude/malli/tree/babashka#babashka

If you will release 0.8.9 then I will release a new version of babashka. It doesn't matter who releases first.

@ikitommi ikitommi merged commit 5ac623f into metosin:master Jul 1, 2022
@ikitommi
Copy link
Member

ikitommi commented Jul 1, 2022

💯

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.

Not working with babashka
2 participants