[anaconda][master][PATCH] Parse boot options before parsing CLI options (#1101341)

Martin Kolman mkolman at redhat.com
Wed May 28 16:54:24 UTC 2014


On Wed, 2014-05-28 at 12:22 -0400, Anne Mulhern wrote:
> > > >              if option.nargs == 0 and option.const is not None:
> > > >                  # nargs == 0 & constr == True -> store_true
> > > > @@ -224,11 +221,13 @@ class AnacondaArgumentParser(ArgumentParser):
> > > >          :returns: an argparse Namespace instance
> > > >          :rtype: Namespace
> > > >          """
> > > > -        # parse arguments (if any) and return the resulting namespace
> > > > -        namespace = ArgumentParser.parse_args(self, args)
> > > > -        # now parse boot options (if any) and modify the namespace
> > > > accordingly
> > > > -        namespace = self.parse_boot_cmdline(boot_cmdline, namespace)
> > > > -        # and return the resulting namespace
> > > > +        # parse boot options first
> > > > +        namespace = self.parse_boot_cmdline(boot_cmdline)
> > > > +        # parse CLI arguments (if any) and add them to the namespace
> > > > +        # created from parsing boot options, overriding any options
> > > > +        # with the same destination already present in the namespace
> > > > +        # NOTE: this basically means that CLI options override boot
> > > > options
> > > 
> > > I'ld leave out the note. When you say something "basically" does something,
> > > you're implying that there are insidious, non-basic, ways in which
> > > it does something else. If that's really true, but I don't think it is,
> > > then the comment should really explain those instead.
> > > 
> > > To be honest, I'ld leave out all the comments here. You shouldn't feel the
> > > need to work so hard just to save me from having to read the parse_args
> > > documentation.
> > Well, I think the way how the boot option and CLI option parsing works &
> > interacts is a bit too tricky to "get" from just reading the source code
> > and therefore I think the additional documentation is needed here.
> > 
> > > 
> > > Surprisingly, the method comment already states that program cmdline
> > > options
> > > override boot cmdline arguments.
> > > 
> 
> Sorry to be pedantic, but...
> 
> There is already a comment in the method header that states
> "Program cmdline arguments will override boot cmdline arguments."
> without any qualification.
> 
> Here's Google's take on 'basically':
> Definition: in the most essential respects; fundamentally.
> Usage: used to indicate that a statement summarizes the most important aspects, or gives a roughly accurate account, of a more complex situation
> So, when I see 'basically' I wonder what actually is going on in this
> more complex situation and why the comment doesn't help me
> by explaining the tricky, complex part, which it hints at so disturbingly.
> 
> By qualifying its main statement with 'basically'
> the note contradicts the method header by implying
> that there is much more to the story than what seems apparent from
> the source code (while the method header states that the code is doing
> what it looks like its doing).
> 
> So, either the header comment or the note is misleading, and should be fixed.
So I'm removing "basically" locally to sync the NOTE: with the
docstring.
> 
> - mulhern
> _______________________________________________
> anaconda-patches mailing list
> anaconda-patches at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/anaconda-patches




More information about the anaconda-patches mailing list