[Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add

[Petr Vobornik]

On 13.11.2014 16:19, Nathaniel McCallum wrote:
> On Thu, 2014-11-13 at 16:13 +0100, Petr Vobornik wrote:
>> On 13.11.2014 16:00, Nathaniel McCallum wrote:
>>> On Thu, 2014-11-13 at 13:17 +0100, Martin Kosek wrote:
>>>> On 11/13/2014 07:53 AM, Nathaniel McCallum wrote:
>>>>> On Thu, 2014-11-06 at 15:33 -0500, Nathaniel McCallum wrote:
>>>>>> This is possible because python-qrcode's output now fits in a standard
>>>>>> terminal. Also, update ipa-otp-import and otptoken-add-yubikey to
>>>>>> disable QR code output as it doesn't make sense in these contexts.
>>>>>>
>>>>>> https://fedorahosted.org/freeipa/ticket/4703
>>>>>
>>>>> I removed the period from the doc string to maintain consistency with
>>>>> the rest of the plugin.
>>>>>
>>>>> Nathaniel
>>>>>
>>>>
>>>> I am not reviewing, just curious. What is the purpose of --qrcode option with
>>>> default=True?
>>>>
>>>>        takes_options = LDAPCreate.takes_options + (
>>>> -        Flag('qrcode?', label=_('Display QR code')),
>>>> +        Flag('qrcode', label=_('Display QR code'), default=True),
>>>>        )
>>>>
>>>> How can user turn it off? Both passing no option and passing --qrcode will get
>>>> the same result, no? :-)
>>>>
>>>> Shouldn't we instead change --qrcode to --no-qrcode and process somehow fill
>>>> qrcode=0 when it's enabled? (To achieve API compatibility with old clients).
>>>
>>> I see three options:
>>> 1. Don't let the user turn it off from the command line (he can still
>>> turn it off from the Python API).
>>>
>>> 2. Change it to --no-qrcode (API change)
>>>
>>> 3. Change the type to Bool (API change)
>>>
>>> Like you, I like #2 the best. Attached is an implementation.
>>
>> I like --no-qrcode as well.
>>
>> Should we also keep qrcode as 'no_option' to maintain API compatibility
>> (but not CLI)?
>
> I don't think it is necessary. It only makes sense to specify --qrcode
> in an interactive session.
>

Makes sense.

ACK

Not pushing yet to give time for NACK if anybody doesn't agree with the 
API change.
-- 
Petr Vobornik