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

Re-think Tagged #148

Closed
Grinkers opened this issue Feb 15, 2024 · 3 comments
Closed

Re-think Tagged #148

Grinkers opened this issue Feb 15, 2024 · 3 comments

Comments

@Grinkers
Copy link
Collaborator

Grinkers commented Feb 15, 2024

Original discussion in #141. I'm open to ideas. This will come after #95 and #141 for sure. I will let this sit in the back of my mind for some weeks/months. This seems really deep, like it might end up needing to almost reimplement clojure.

Off-spec

https://github.com/edn-format/edn?tab=readme-ov-file#tagged-elements
I think the current implementation doesn't follow the spec. I believe we shouldn't have a Tagged type at all (and certainly not a (String, Edn)). An interesting thing to note is that I believe this is lossy. In my #foo and #bar examples, after the read-string, the tag is actually lost

reading a tag and tagged element yields one value.

https://github.com/edn-rs/edn-rs/blob/d64f20c3079268372970398349ae5bf5b7b40f41/tests/deserialize.rs#L716
This should throw an error without any custom readers.

(clojure.edn/read-string "#keyword :4")
java.lang.RuntimeException: No reader function for tag keyword [at <repl>:1:1]

Idea 1 (seems bad)

Clojure has a couple ways to define custom readers. One can be seen https://clojuredocs.org/clojure.core/data-readers Rust doesn't have a great way to dynamically do this. That would require creating Edn instances, mutating/registering them, pointers/lifetimes, etc. It would be a horrible mess to use.

Idea 2

The other way can be seen in this example

(require '[clojure.edn :as edn])

(defn foo [i] (if (= i 41) 42 "foobar"))
(defn bar [i] (str i "bar"))

;; prints "foobar"
(println (edn/read-string {:readers {'foo foo 'bar bar}} "#foo bar"))
;; prints "foobar"
(println (edn/read-string {:readers {'foo foo 'bar bar}} "#bar foo"))
;; prints "42"
(println (edn/read-string {:readers {'foo foo 'bar bar}} "#foo 41"))

I think we could do something like this in rust

fn foo(edn: (str, Edn)) -> T1 {
  match edn.0 {
    Edn::UInt(41) => 42,
    _ => "foobar".to_owned(),
  }
}

fn uui(edn: (str, Edn)) -> T2 {
   // some custom library/impl/whatever
   uuid::parse(edn.1)
}

fn custom_parse(): -> F 
where
  F: FnOnce(str) -> Result<GeneratedSumTypeForTs>, 
{
  edn_rs::some_new_fancy_macro!(foo, bar, more, readers)
}

// users can register a custom fn and then use it
let custom_reader_from_str = custom_parse();
let edn = custom_reader_from_str("#foo 41");

Idea 3

Agree this is insane and just keep the current implementation and leave it as an exercise for the user. Alternatively, we can feature gate Tagged, so people who need it to not be lossy can opt-in, but mark it as unstable/off-spec.

@Grinkers
Copy link
Collaborator Author

Grinkers commented Feb 15, 2024

Wow, TIL clojure's read-string is actually quite powerful. It really is fully Extensible.

(require '[clojure.edn :as edn]
         '[clojure.string :as str]
         '[cheshire.core :as json])

(defn parse-rust-nums [s]
  (as-> s a
    (str/split a #"_")
    (apply str a)
    (Integer/parseInt a)))
(def parsed-data
  (edn/read-string
   {:readers {'rust parse-rust-nums 'json #(json/parse-string %) 'keyword #(keyword %)}}
   "[#json \"{\\\"cat\\\": 42}\" #rust \"123_456_221\" #keyword \"keyword with spaces\"]"))

;; (clojure.lang.PersistentArrayMap java.lang.Integer clojure.lang.Keyword)
(prn (map type parsed-data))
;; [{"cat" 42} 123456221 :keyword with spaces]. 
(prn parsed-data)

note that clojure actually supports keywords with spaces in them, even though it's not normally possible to type them in a repl directly with : or edn-able with prn-str (this is a particular clojure pet peeve of mine). A little custom reader can add support for this.

@naomijub
Copy link
Owner

Will try to read with more attention this weekend

@Grinkers
Copy link
Collaborator Author

#158

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

No branches or pull requests

2 participants