search

kyleboe / zoom_rb

Ruby REST API Wrapper for zoom.us API
https://developers.zoom.us/docs/api/
MIT License
83 stars 104 forks source link

Attempt refactor #392

Closed joelmichael closed 2 years ago

joelmichael commented 3 years ago

Creates lib/zoom/actions.rb (Zoom::ActionDefinitions). Reworks Zoom::Actions::Account to use new define_action method.

joelmichael commented 3 years ago

This is just a proof-of-concept. The names of the modules, methods, and how they're integrated are all up in the air. We could use keyword arguments on define_action for example.

benjaminwood commented 3 years ago

I like the direction here! @kyleboe has more context and may be able to tell if there are endpoints where this doesn't work.

If we all agree this is a good path forward, then I vote you write specs for Zoom::ActionDefinitions and we merge this in, then follow up with small PR's to migrate the remaining actions to use Zoom::ActionDefinitions.

One other thought - what about tests for the actions themselves? Our tests are all pretty much the same, and there is a lot of boilerplate involved in setting them up. I question what value they actually have. Any ideas?

gerryster commented 2 years ago

As someone who has been using this gem a lot recently and peering into its internals, I also like the direction of this refactor. There is a lot of boilerplate in the actions and a more declarative approach would help.

joelmichael commented 2 years ago

I think I was able to successfully migrate all actions to a new system except user_create, which has currently been configured to differ from the actual API.

benjaminwood commented 2 years ago

Great work @joelmichael! This is looking so much better.

If we add thorough specs for Zoom::ActionDefinitions, I feel we could simply remove all the specs for the individual API endpoints (and thus adding new endpoints would be super easy).

joelmichael commented 2 years ago

Breaking change

I was able to migrate the user_create action to the new system, but with limited and altered functionality compared to the old way. This change must be documented for users of this gem upon release of this refactor as a new major version, or it will certainly break their user_create calls.

joelmichael commented 2 years ago

@kyleboe Please take a look at this when you get a chance. I intend to write some tests for it but would like to merge it maybe next week or the week after.