gfredericks / vcr-clj

Generic IO playback for clojure
117 stars 20 forks source link

Serialisation Options #26

Closed sbreatnach closed 6 years ago

sbreatnach commented 6 years ago

I've added optional arguments to support extending or modifying the default behaviour of cassette serialisation. This was to allow me to change the default behaviour of byte array serialisation, which is safe by default but means re-recording the cassette each time even one minor bit of the response changes. Changing it to simply save the stringified version makes the cassette easy to modify manually in those cases. Also, I want to add correct de/serialisation of exceptions for some tests in future and this will allow me to do so.

I've also added a new macro that exposes these serialisation arguments as a separate required argument. I haven't documented this in the README as yet because I'm not sure if this is what you want to do for exposing these options in the public interface. My own tests don't use this but use a more custom macro that isn't really suitable here.

gfredericks commented 6 years ago

hello, thanks for working on this! I agree that being able to control serialization is useful. I think it could be done simply by supporting the :print-handlers and :data-readers options on the spec maps passed to with-cassette; would that work for you?

sbreatnach commented 6 years ago

That's actually the approach I initially undertook, but then I realised it didn't make much sense because you can pass a list of specs. So if you pass the :data-readers as part of every spec, which one do you choose? What if the client code passes :print-handlers for one spec but not the other? It only makes sense if there would be a separate cassette file for each spec but all the specs are recorded into one cassette file as it stands. That's why I made it an optional fourth argument to with-cassette-fn*.

It might be possible to do some trickery in with-cassette to add an optional third argument without breaking backwards compatibility but I didn't want to go down that rabbit hole. I tend to avoid writing macros if at all possible since I find they quickly become an unreadable squiggle. As such, my macro-fu is fairly weak.

Another option is leave the macros be and I can document an 'Advanced Usage' section that explains using with-cassette-fn* directly. It's not a common thing to do anyway, changing serialisation options in any meaningful way.

gfredericks commented 6 years ago

Well dangit you're right, that doesn't make sense.

Having cassette-level options could be useful in other ways too.

I was convinced the backwards-compatible trickery should be possible, but actually I don't think it is, at least without requiring something like map literals for the opts arg, which would be terrible. The problem is that the macro has to decide what parts to evaluate at what time, and if it can't tell if an optional arg is an opts map without evaluating it, then it might end up evaluating part of the body too early.

~What if we pulled a rich hickey and added a with-cassette-2 macro where the second arg is~ Hey I've got an idea! The first arg, the cassette name, could be allowed to be a map. I like that, because the cassette name is already cassette-level information.

(with-cassette {:name "test-the-frobnicator" :data-readers {...}} 
               [{...}]
   ...)

Or we could do the with-cassette-2 thing. What do you think?

sbreatnach commented 6 years ago

Making the first argument either a string or a map with a name string is the best solution to me. It's backwards-compatible and a common pattern in many Clojure libs. I'll make the changes and document them as well.

sbreatnach commented 6 years ago

Ok, I've updated based on your feedback. With the updated test, I simply used a spy on the print function since anything more complex is testing puget functionality as much as vcr-clj. I also added in a basic validation check for the cassette name. Not sure if that is overstepping the mark or whether you would prefer a different method of validation.

gfredericks commented 6 years ago

looks good, thanks!

I have another minor change planned in the next week or so, so I'll make a release after that.

gfredericks commented 6 years ago

This was just released in 0.4.17.

sbreatnach commented 6 years ago

Great, thanks!