[asterisk-dev] [Code Review] 2940: ARI: User better nicknames for ARI operations

David Lee reviewboard at asterisk.org
Mon Oct 21 12:03:22 CDT 2013 


> On Oct. 18, 2013, 5:51 p.m., Paul Belanger wrote:
> > Cool, I ran into the same issue while doing python-ari.  Below are some suggestions / recommendation on how I named things.
> > 
> > But in general, I think we should use something like the following
> > 
> > get (single)
> > create (POST)
> > delete (DELETE)
> > list (multiple)
> > 
> > add (toggle)
> > remove (toggle)
> > 
> > addSomething (toggle)
> > removeSomething (toggle)
> > 
> > Thoughts?
> > 
> >

list is probably better than getAll, esp. for cases where you can filter the responses.

In some languages, delete is a reserved word, hence my use of destroy.

Using add/remove for toggles (hold, musing on hold, etc.) looks really off. You're not "adding" anything to the object, you're changing its state. "channel.hold()" looks much more natural than "channel.addHold()".


> On Oct. 18, 2013, 5:51 p.m., Paul Belanger wrote:
> > /branches/12/rest-api/api-docs/bridges.json, line 76
> > <https://reviewboard.asterisk.org/r/2940/diff/1/?file=47296#file47296line76>
> >
> >     I prefer delete to destory, seems less violent.

I wanted to avoid 'delete', because it's a keyword in some languages.


> On Oct. 18, 2013, 5:51 p.m., Paul Belanger wrote:
> > /branches/12/rest-api/api-docs/bridges.json, line 208
> > <https://reviewboard.asterisk.org/r/2940/diff/1/?file=47296#file47296line208>
> >
> >     This might be for another review, but what about just addMusic?
> >     
> >     moh is not very description and add to the fact we already have a hold function.

That would be another discussion. I'd want the nickname to match the path, so we'd want to change both.


> On Oct. 18, 2013, 5:51 p.m., Paul Belanger wrote:
> > /branches/12/rest-api/api-docs/bridges.json, line 249
> > <https://reviewboard.asterisk.org/r/2940/diff/1/?file=47296#file47296line249>
> >
> >     removeMusic

Same.


> On Oct. 18, 2013, 5:51 p.m., Paul Belanger wrote:
> > /branches/12/rest-api/api-docs/channels.json, line 410
> > <https://reviewboard.asterisk.org/r/2940/diff/1/?file=47297#file47297line410>
> >
> >     We see to flip flop between hold / unhold and addChannel / removeChannel.  What about just using add / remove everywhere?
> >     
> >     
> >     addHold / removeHold seems more consistent

I don't see this as 'adding' hold to the channel. We're putting the channel on hold.


> On Oct. 18, 2013, 5:51 p.m., Paul Belanger wrote:
> > /branches/12/rest-api/api-docs/channels.json, line 131
> > <https://reviewboard.asterisk.org/r/2940/diff/1/?file=47297#file47297line131>
> >
> >     would rather keep this as delete to keep things consistent.  Otherwise, we should move this to the /hangup action.

There's tension between how one would organize a RESTful API (which is
how you setup paths and HTTP methods) and an OO API (which is where
the nicknames would be used).

We'll occasionally find impedance mismatches such as these, where what
you would want to put on the OO API (channel.hangup()) doesn't match
what you would want in the REST API (DELETE /channels/{id}). Since the
nickname gives you flexability, we don't have to compromise one for
the other.


> On Oct. 18, 2013, 5:51 p.m., Paul Belanger wrote:
> > /branches/12/rest-api/api-docs/channels.json, line 475
> > <https://reviewboard.asterisk.org/r/2940/diff/1/?file=47297#file47297line475>
> >
> >     same comment as above

Same response.


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2940/#review9955
-----------------------------------------------------------


On Oct. 18, 2013, 4:09 p.m., David Lee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2940/
> -----------------------------------------------------------
> 
> (Updated Oct. 18, 2013, 4:09 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> While working on building client libraries from the Swagger API, I
> noticed a problem with the nicknames.
> 
>     channel.deleteChannel()
>     channel.answerChannel()
>     channel.muteChannel()
> 
> Etc. We put the object name in the nickname (since we were generating C
> code), but it makes OO generators redundant.
> 
> This patch makes the nicknames more OO friendly. This resulted in a lot
> of name changing within the res_ari_*.so modules, but not much else.
> 
> There were a couple of other fixed I made in the process.
> 
>  * When reversible operations (POST /hold, POST /unhold) were made more
>    RESTful (POST /hold, DELETE /unhold), the path for the second operation
>    was left in the API declaration. This worked, but really the two
>    operations should have been on the same API.
>  * The POST /unmute operation had still not been REST-ified.
> 
> 
> Diffs
> -----
> 
>   /branches/12/res/ari/resource_applications.h 401260 
>   /branches/12/res/ari/resource_applications.c 401260 
>   /branches/12/res/ari/resource_asterisk.h 401260 
>   /branches/12/res/ari/resource_asterisk.c 401260 
>   /branches/12/res/ari/resource_bridges.h 401260 
>   /branches/12/res/ari/resource_bridges.c 401260 
>   /branches/12/res/ari/resource_channels.h 401260 
>   /branches/12/res/ari/resource_channels.c 401260 
>   /branches/12/res/ari/resource_endpoints.h 401260 
>   /branches/12/res/ari/resource_endpoints.c 401260 
>   /branches/12/res/ari/resource_events.h 401260 
>   /branches/12/res/ari/resource_events.c 401260 
>   /branches/12/res/ari/resource_playback.h 401260 
>   /branches/12/res/ari/resource_playback.c 401260 
>   /branches/12/res/ari/resource_recordings.h 401260 
>   /branches/12/res/ari/resource_recordings.c 401260 
>   /branches/12/res/ari/resource_sounds.h 401260 
>   /branches/12/res/ari/resource_sounds.c 401260 
>   /branches/12/res/res_ari_applications.c 401260 
>   /branches/12/res/res_ari_asterisk.c 401260 
>   /branches/12/res/res_ari_bridges.c 401260 
>   /branches/12/res/res_ari_channels.c 401260 
>   /branches/12/res/res_ari_endpoints.c 401260 
>   /branches/12/res/res_ari_events.c 401260 
>   /branches/12/res/res_ari_playback.c 401260 
>   /branches/12/res/res_ari_recordings.c 401260 
>   /branches/12/res/res_ari_sounds.c 401260 
>   /branches/12/rest-api-templates/ari_resource.c.mustache 401260 
>   /branches/12/rest-api-templates/ari_resource.h.mustache 401260 
>   /branches/12/rest-api-templates/asterisk_processor.py 401260 
>   /branches/12/rest-api-templates/res_ari_resource.c.mustache 401260 
>   /branches/12/rest-api-templates/rest_handler.mustache 401260 
>   /branches/12/rest-api-templates/swagger_model.py 401260 
>   /branches/12/rest-api/api-docs/applications.json 401260 
>   /branches/12/rest-api/api-docs/asterisk.json 401260 
>   /branches/12/rest-api/api-docs/bridges.json 401260 
>   /branches/12/rest-api/api-docs/channels.json 401260 
>   /branches/12/rest-api/api-docs/endpoints.json 401260 
>   /branches/12/rest-api/api-docs/playback.json 401260 
>   /branches/12/rest-api/api-docs/recordings.json 401260 
>   /branches/12/rest-api/api-docs/sounds.json 401260 
> 
> Diff: https://reviewboard.asterisk.org/r/2940/diff/
> 
> 
> Testing
> -------
> 
> Hit each ARI resource to ensure it still responded.
> 
> 
> Thanks,
> 
> David Lee
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131021/568a82f1/attachment-0001.html>

More information about the asterisk-dev mailing list