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

feat: accept vectors of size 2 in addition to MapEntry in unparse #1140

Closed
wants to merge 3 commits into from

Conversation

opqdonut
Copy link
Member

fixes #1123

@opqdonut opqdonut requested a review from ikitommi November 28, 2024 09:06
(if-some [unparse (get unparsers (key x))]
(unparse (val x))
(if-some [unparse (get unparsers (first x))]
(unparse (second x))
Copy link
Collaborator

Choose a reason for hiding this comment

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

nth might be more equivalent in performance here since I think first needs to go via seq.

@ikitommi
Copy link
Member

ikitommi commented Nov 28, 2024

(def schema
  [:or
   [:tuple :string :keyword]
   [:orn ["any" :keyword]]])

(->> (m/parse schema :any) (m/unparse schema))
; => ["any" :any]

@opqdonut
Copy link
Member Author

Ouch, that's a nice corner-case! This behaviour is the same in master since a MapEntry is a :tuple. The safest solution would be to switch to something like a custom record, but that wouldn't help users solve the original problem (of fabricating inputs to unparse easily).

;; in master
(m/validate [:tuple :string :keyword] (clojure.lang.MapEntry. "a" :a)) ; => true

@ikitommi
Copy link
Member

Indeed. So, to get correct behavior, we would need the record. Maybe for the :catn case too where we use regular map now? If the record would implement right interfaces / protocols, this would not even be a breaking change?

@ikitommi
Copy link
Member

.. but would this solve the original issue?

@ikitommi
Copy link
Member

ikitommi commented Nov 30, 2024

(defrecord Tagged [key value])

(defn -tagged [key value] (->Tagged key value))
(defn -tagged? [x] (instance? Tagged x))
  • changing access to (:key tagged) and (:value tagged):
(def schema
  [:or
   [:tuple :string :keyword]
   [:orn ["any" :keyword]]])

(->> :any (m/parse schema))
; => #malli.impl.util.Tagged{:key "any", :value :any}

(m/unparse schema *1)
; => :any

(m/unparse schema (miu/-tagged "any" :any))
; => :any

I think this would solve this and no ambiguity with vectors.

@opqdonut
Copy link
Member Author

opqdonut commented Dec 2, 2024

The record wouldn't fully solve the original problem (making custom inputs to unparse easy), but would at least solve the footgun (things look like vectors but aren't). I agree that the record would be the most correct solution.

Re: breaking, I think it would be breaking for people who are consuming the output of parse.

@ikitommi
Copy link
Member

ikitommi commented Dec 8, 2024

Re: breaking, I think it would be breaking for people who are consuming the output of parse.

true that. Tried to look if the parse syntax is agreed to be stable or not, no mention of that so user can assume it is, thus breaking.

opqdonut added a commit that referenced this pull request Dec 16, 2024
Using a MapEntry was confusing users, because it printed like a
vector, but you couldn't give a vector to unparse.

The current method of using MapEntry was broken for weird schemas:

```
(def schema
  [:or
   [:tuple :string :keyword]
   [:orn ["any" :keyword]]])

(->> (m/parse schema :any) (m/unparse schema))
; => ["any" :any]
; should've been :any
```

Changes the parse behaviour for (at least) :orn, :altn and :multi

Some place (like the entry parsers) used miu/-tagged to generate
MapEntry values. These use sites now use the new miu/-entry. This
keeps the surface area of this change a lot smaller since we don't
need to touch all the map entry logic.

fixes #1123
replaces #1140
@opqdonut
Copy link
Member Author

The record solution is now available in #1150 . Closing this PR.

@opqdonut opqdonut closed this Dec 16, 2024
opqdonut added a commit that referenced this pull request Dec 16, 2024
Using a MapEntry was confusing users, because it printed like a
vector, but you couldn't give a vector to unparse.

The current method of using MapEntry was broken for weird schemas:

```
(def schema
  [:or
   [:tuple :string :keyword]
   [:orn ["any" :keyword]]])

(->> (m/parse schema :any) (m/unparse schema))
; => ["any" :any]
; should've been :any
```

Changes the parse behaviour for (at least) :orn, :altn and :multi

Some place (like the entry parsers) used miu/-tagged to generate
MapEntry values. These use sites now use the new miu/-entry. This
keeps the surface area of this change a lot smaller since we don't
need to touch all the map entry logic.

fixes #1123
replaces #1140
opqdonut added a commit that referenced this pull request Jan 7, 2025
Using a MapEntry was confusing users, because it printed like a
vector, but you couldn't give a vector to unparse.

The current method of using MapEntry was broken for weird schemas:

```
(def schema
  [:or
   [:tuple :string :keyword]
   [:orn ["any" :keyword]]])

(->> (m/parse schema :any) (m/unparse schema))
; => ["any" :any]
; should've been :any
```

Changes the parse behaviour for (at least) :orn, :altn and :multi

Some place (like the entry parsers) used miu/-tagged to generate
MapEntry values. These use sites now use the new miu/-entry. This
keeps the surface area of this change a lot smaller since we don't
need to touch all the map entry logic.

fixes #1123
replaces #1140
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Clarify and maybe change unparse behavior
3 participants