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

WIP: :-> #1027

Merged
merged 34 commits into from
Jun 30, 2024
Merged

WIP: :-> #1027

merged 34 commits into from
Jun 30, 2024

Conversation

frenchy64
Copy link
Contributor

@frenchy64 frenchy64 commented Mar 28, 2024

Introduce a flat syntax for function arguments.

[:-> :any] ; [:=> :cat :any]
[:-> :int :any] ; [:=> [:cat :int] :any]
[:-> [:cat :int] :any]  ; [:=> [:cat [:cat :int]] :any]
[:-> a b c d :any] ; [:=> [:cat a b c d] :any]

;; guard property
[:-> {:fn (fn [[[arg] ret]] ...)}
 :arg :ret]
; [:=> [:cat :arg] :ret [:fn (fn [[[arg] ret]] ...)]]
  • disallow :-> without children, looks like an infix op [:=> :a :-> :b].
  • test :fn property
  • add generators for :ifn :->

Copy link
Member

@ikitommi ikitommi left a comment

Choose a reason for hiding this comment

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

Thanks! I also too a stab at this earlier. Looking at my stash, the idea was:

  • move the malli.util/-util-schema as malli.core/-proxy-schema
  • implement :-> using that (you can m/deref to get the underlaying :=> of this)
  • there are few places where one needs to check "if the schema is a function schema". There could be a new FunctionSchema protocol or some other & simpler way to tag both variants as valid.
  • I just listed both e.g. in :function schema (oh, the crappy name of that!)
-        (when-not (every? #(= :=> (type %)) children)
+        (when-not (every? #(= #{:=> :->} (type %)) children)

the actual :-> impl was:

(defn -->-schema []
  (-proxy-schema {:type :->
                  :fn (fn [p c o] (let [c (map #(schema % o) c), cc [(into [:cat] (butlast c)) (last c)]]
                                    [c (map -form c) (into-schema :=> p cc o)]))}))

not sure if that worked in all possible places, was a quick test, see https://clojurians.slack.com/archives/CLDK6MFMK/p1706446416126059

I think having a new Schema would be simpler here. WDYT?

@@ -1760,9 +1760,28 @@
(-type [_] :=>)
Copy link
Member

Choose a reason for hiding this comment

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

this should be changed too

@@ -1749,7 +1749,7 @@
{:min 1 :max 1}
(-regex-min-max child nested?)))))))))

(defn -=>-schema []
(defn -=>-schema [{:keys [flat-arrow?]}]
Copy link
Member

Choose a reason for hiding this comment

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

keys with ? in the end should be predicate functions, so flat-arrow would suffice here. https://github.com/bbatsov/clojure-style-guide?tab=readme-ov-file#naming-predicates

@frenchy64
Copy link
Contributor Author

@ikitommi I took your idea and ran with it. Since you were unhappy with :function, I added a new proxy schema :ifn. LMK what you think.

[:function [:=> [:cat] :int] [:=> [:cat :int] :int]]
==
[:ifn [:-> :int] [:-> :int :int]]

@ikitommi
Copy link
Member

Hey, finally had time to dig into this. I think the :ifn ~= :function gives too many ways to do one thing:

[:function [:=> :cat :int]]
[:function [:-> :int]]
[:ifn [:=> :cat :int]]
[:ifn [:-> :int]]

also, :ifn sound bit like ifn? which is a different thing, e.g. a map gives true to that.

otherwise, looks good! I'll pick up this branch and play with it + add if something is missing or would like to have it differently.

…implicity), add tests

* NOTE1: :-> explain shows the details
* NOTE2: :-> ast is not perfect
@ikitommi
Copy link
Member

I dropped the AritySchema and moved the -function-info into the Function protocol + added some tests + renamed :fn property as :guard.

Few things I noticed:

  1. explain now explains on underlaying :=> schema, might be ok?
  2. there is no custom ast & to-ast for :->

@ikitommi ikitommi marked this pull request as ready for review June 30, 2024 13:12
@ikitommi
Copy link
Member

I dropped :-> from the default registry and marked m/-->-schema as experimental due to AST and explain results potentially being unstable. But, good to go to master.

@ikitommi ikitommi merged commit 3cc8116 into metosin:master Jun 30, 2024
9 checks passed
@ikitommi
Copy link
Member

Thanks!!

@frenchy64
Copy link
Contributor Author

also, :ifn sound bit like ifn? which is a different thing, e.g. a map gives true to that.

I don't see the problem, that's also true for :function.

user=> (m/validate [:function [:=> [:cat [:enum 1]] [:enum 1]]] {1 1})
true

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.

None yet

2 participants