clj-commons/clj-yaml

Accidentally broke decode?

lread opened this issue · 8 comments

lread commented

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

  • Confirm we did release a breaking change, see #66 for example projects in the wild.
  • If so, try to adapt clj-yaml to make this a non-breaking change for both the last release and the previous releases.
borkdude commented

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

lread commented

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

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

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

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

borkdude commented

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

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.

  • 👍1
lread commented

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:

  • untouched project tests pass
  • project tests fail after upgrading to clj-yaml v0.7.169
  • project tests pass after upgrading to clj-yaml master