search

clj-commons / clj-yaml

YAML encoding and decoding for Clojure
Other
122 stars 26 forks source link

Accidentally broke decode? #67

Closed lread closed 2 years ago

lread commented 2 years ago

Issue?

We just recently released a version of clj-yaml that included a change to the decode protocol method function signature.

At the time we really did not understand that folks were using clj-yaml at this level and assumed this was internal only code. But since then I have had a look as part of #66.

So I think we might have released a breaking change without realizing it.

Next Steps

borkdude commented 2 years ago

Now that the break has happened, we could wait for people to complain and then fix it in a new version.

lread commented 2 years ago

Yeah true, but if I can find a way to easily be proactive, might proceed. But am feeling a bit clj-yaml weary, so might wait! 🙂 At the very least I'll verify if I'm correct about the break. Think so.

lread commented 2 years ago

TLDR: Confirmed that we broke muuntaja.

Repro

on macOS using JDK8...

Clone

$ git clone git@github.com:metosin/muuntaja.git
$ cd muuntaja

Sanity test before we make changes:

$ lein test
...
Ran 54 tests containing 345 assertions.
0 failures, 0 errors.

Bump clj-yaml from 0.7.106 to 0.7.169 in project.clj (our current release):

$ lein clean
$ lein test
...
Syntax error (IllegalArgumentException) compiling yaml/decode at (muuntaja/format/yaml.clj:14:7).
No single method: decode of interface: clj_yaml.core.YAMLCodec found for function: decode of protocol: YAMLCodec

Yep, ok, as expected decode fn signature change caused breakage.

lread commented 2 years ago

Old YAMLCodec sig: (decode [data keywords]) New YAMLCodec sig: (decode [data keywords unknown-tag-fn])

Of course, we would have preferred an extensible (decode [data opts]) but that's not how this was designed.

Not well-thought-out yet, but initial ideas:

  1. can we move to (decode [data opts]) somehow?
    1. YAMLCodec2?
    2. Break 0.7.169 only and reinterpret 2nd arg as opts or keywords based on type (boolean vs map)?
  2. should we try to support both decode sigs with a multi-arity decode? Or is that just too plain awkward?
lread commented 2 years ago

Updated changelog marking v0.7.169 as "minor breaking".

borkdude commented 2 years ago

My preference would be 1.ii. 0.7.169 was probably not used that much yet, especially on this lower level of protocol usage. So we could do (decode [data opts]) with backward compatibility for keywords.

lread commented 2 years ago

Thanks @borkdude, after sleeping on it, I think that's the way to go too.

Probably better to get done sooner than later, so I'll see if I can get this done my-today and publish a new release.

lread commented 2 years ago

Verified projects I noticed using decode in #66:

  • kvert is using what I guessed was internal. It extends the YAMLCodec, makes use of make-yaml, decode (we just effectively broke this API with our recent unknown tag support).

  • I think we also broke muuntaja with our decode signature change. They use encode, decode, makeyaml.

  • swarmpit also uses YAMLCodec, encode, and even flow-styles.

Verified: