[asterisk-dev] [Code Review] 3008: app_confbridge: Add an option 'announce_join_leave_review' for users that adds recording review to the 'announce_join_leave' name recording feature

[Mark Michelson]

> On Nov. 11, 2013, 9:54 p.m., Matt Jordan wrote:
> > /trunk/apps/app_confbridge.c, lines 1511-1530
> > <https://reviewboard.asterisk.org/r/3008/diff/1/?file=48128#file48128line1511>
> >
> >     The way you've implemented this, where announce_join_leave_review 'augments' another option, is odd.
> >     
> >     The idea of having one option augment another sounds interesting, but in practice, is probably going to be more frustrating than useful.
> >     
> >     For example, the following won't work:
> >     
> >     [my_user]
> >     type=user
> >     announce_join_leave_review=Yes
> >     
> >     And yet it is a valid user profile that will be loaded. I just won't get the behavior I want.
> >     
> >     These two options should be mutually exclusive of each other. You should only be able to specify one of them, but you should only have to specify one of them to get the behavior you want.
> >     
> >     Alternatively, you could go with a single option with multiple values.

I'm going to throw my two cents in and suggest the use of multiple options rather than allowing multiple values for the same option. If this option were being designed from scratch, it might make sense to word the option in such a way as to allow for multiple values, but changing a boolean into a tri-bool is just weird.


- Mark


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


On Nov. 11, 2013, 8:08 p.m., Jonathan Rose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3008/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2013, 8:08 p.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson and rmudgett.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Pretty straight forward really.  Adds the same basic functionality provided by the 'i' option to MeetMe into Confbridge (which already implemented functionality similar to the 'I' option through announce_join_leave)
> 
> 
> Diffs
> -----
> 
>   /trunk/configs/confbridge.conf.sample 402603 
>   /trunk/apps/confbridge/include/confbridge.h 402603 
>   /trunk/apps/confbridge/conf_config_parser.c 402603 
>   /trunk/apps/app_confbridge.c 402603 
>   /trunk/CHANGES 402603 
> 
> Diff: https://reviewboard.asterisk.org/r/3008/diff/
> 
> 
> Testing
> -------
> 
> Made sure the recording feature worked with and without the review option in the expected way (with and without recording review). It's a pretty straight forward patch though.
> 
> 
> Thanks,
> 
> Jonathan Rose
> 
>

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