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

[Nathaniel McCallum]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-npmccallum-0078.1-Enable-QR-code-display-by-default-in-otptoken-add.patch
Type: text/x-patch
Size: 4761 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20141113/539ce4d1/attachment.bin>