Refactor cable_car to eliminate duplication

[joshleblanc]

2 files added:

• operation_builder.rb
  • Holds all duplicate code between channel and cable_car. This class is solely responsible for building the operations hash, and returning the required values for transmitting and such
• operational.rb
  • Operational is a module included by classes using OperationBuilder. It exposes a operation_builder method that creates or returns a single instance of OperationBuilder. It forwards all missing methods to OperationBuilder should it respond to it, as well as implicitly returns self from missing methods (allowing method chaining).

I also added tests!

[leastbad]

Hey Josh, I finally had a proper opportunity to go over this PR and it's really great. I have some questions and requests but I'm 100% going to merge this once it's sorted.

1. Nate changed the way the finalizers are handled. Could you fix the merge conflict and make sure all of the classes follow the pattern he set out? (Specifically, I think operation_builder.rb will need to be adapted.)
2. What's the purpose of the method missing construct in operational.rb - that is, why is this necessary?
3. What is the purpose of the _ = nil parameter definition in the to_json method of operation_builder.rb?
4. Any chance you'd be up for implementing the custom ApplicationController Renderer in cable_ready.rb? You expressed a confidence that I just don't have, and I fully believe you're probably right about how it could work.
5. I see that you rolled back my proposed deep_transform_keys! fix for the options hash. You're probably right about this not being necessary since my tests today show that the JSON generated is consistent even without the deep_transform_keys! call. You can see in the example below that the @enqueued_operations has bonusRound and bonus_round keys, but it all seems to come out fine:

Thanks Josh! This makes the whole PR even more transformative.

[joshleblanc]

@leastbad

The method_missing in the operation module forwards calls to the underlying operation builder. Lets you use the operations directly on cable_car/channel. It's basically a generic way to delegate methods to the operation builder.

to_json accepts a single parameter, so when we're defining our own we should respect that standard. Really I should be forwarding `args to the underlying to_json, so I'll do that when I work out those merge conflicts and check out the custom renderer.

[leastbad]

You rock, sir! I was very impressed with your implementation.

[joshleblanc]

@leastbad

Nate's changes threw a wrench in the works, so I've made OperationBuilder a class that cable car and channel inherit. It's functionally the same as it was, except now cable_car has an identifier of "CableCar".

I've added the custom renderer you were looking for as well.

[leastbad]

That's awesome, thank you very much.

I'm sorry that the finalizer changes were more dramatic than I had realized. That's also precisely why I left it for you.

When you say that "cable_car has an identifier of 'CableCar'", where does that come into play? What I'm really asking/picturing is that developers will include CableReady::Broadcaster and be able to use either cable_ready or cable_car depending on their goals. Is that still true? In what scenario might a developer need to access the CableCar class?

[joshleblanc]

Doesn't effect the developer, it's just for the finalizers.

https://github.com/leastbad/cable_ready/pull/3/files#diff-3b00f39afd756a369ec89f2e16eacefb3e10e6b423cc3e96fcfd294ae436e1f7R10

[leastbad]

channel.rb is now broken because there's no operation_builder in scope.

I'm pretty sure this would fix it:

    def broadcast(clear: true)
      ActionCable.server.broadcast identifier, {"cableReady" => true, "operations" => operations_payload}
      reset! if clear
    end

    def broadcast_to(model, clear: true)
      identifier.broadcast_to model, {"cableReady" => true, "operations" => operations_payload}
      reset! if clear
    end

[joshleblanc]

@leastbad Thanks for the catch.

Had trouble getting tests working for those methods due to not running in a rails context. We should really cover those if we can figure out how.

[leastbad]

Just being honest, @marcoroth is about 850x more likely to know how to help with that. It's a major hole in my capability.

Anyhow, thanks for helping to make cable_car and CableReady better!