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

[Nathaniel McCallum]

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.