Improve guess support in rbt post.

Review Request #5614 — Created March 12, 2014 and submitted

Information

RBTools
master
ee401fd...

Reviewers

This adds new abilities to rbt post's guess fields support. The old
guess support was a boolean state, which only supported opt-in and not
opt-out. That is, if GUESS_FIELDS was set to True in .reviewboardrc, it
couldn't be turned off when posting.

It's now a tri-state of "yes", "no", and "auto", which can be set on the
command line or .reviewboardrc.

If "yes", it behaves as before, with the fields being updated from the
commit message.

If "no", guessing will be turned off (which allows the command line to
override the config file).

If "auto", guessing will be turned on only for new review requests, but
turned off when updating.

If no --guess-* options or configuration variables are set, the
default will be "auto", making for a nicer experience when posting brand
new review requests.

If an old-style --guess-* option is passed without a value (in the form
of --guess-*=value or -gvalue), then the prior logic of forcing guessing
will be used. That is, "yes" is implied.

While "yes", "no", and "auto" are all valid values, we still support
True and False in .reviewboardrc, for backwards-compatibility.

Posted this change by doing rbt post, without any parameters.

Temporarily turned off actual publishing of the change and just outputted
the results of the guess flags. Then I did tests with all three values
in .reviewboardrc and with/without command line arguments and their possible
values. Saw the resulting values that I expected.

Turned posting back on, removed the settings from .reviewboardrc, and posted
a change without explicitly passing any guess arguments. The review request
had the fields filled in from my commit message.

Made changes to the description and summary on my review request, and updated
using -r. The changes weren't overwritten. Tested also with -u.

Then I tried with --guess-fields=no, and it didn't overwrite either.

Tried with yes and it did overwrite.

Tried again without a value and it overwrote again.

Posted a new change with --guess-fields=no and it didn't provide any defaults.

Unit tests pass.

Built the docs, read through, and verified all links.

Description From Last Updated

This should probably mention that it only applies to SCMs where you're posting commits (git, hg, bzr).

daviddavid

I'd prefer to not include this example.

daviddavid

How about printing the value as well?

daviddavid

Maybe 'Invalid value "%s" for argument "%s"' ?

daviddavid

How about "changes from a working directory"? "staged" is only really git terminology.

daviddavid
david
  1. 
      
  2. docs/rbtools/rbt/commands/post.txt (Diff revision 1)
     
     
     
     
     

    This should probably mention that it only applies to SCMs where you're posting commits (git, hg, bzr).

  3. docs/rbtools/rbt/commands/post.txt (Diff revision 1)
     
     

    I'd prefer to not include this example.

    1. I could be more explicit about it, but it's actually a useful one if you want to disable guessing entirely for this post.

      Was it just the short form you didn't like?

    2. Yeah, just the short form.

  4. rbtools/commands/post.py (Diff revision 1)
     
     

    How about printing the value as well?

  5. 
      
chipx86
david
  1. 
      
  2. rbtools/commands/post.py (Diff revisions 1 - 2)
     
     
     

    Maybe 'Invalid value "%s" for argument "%s"' ?

  3. docs/rbtools/rbt/commands/post.txt (Diff revision 2)
     
     

    How about "changes from a working directory"? "staged" is only really git terminology.

  4. 
      
chipx86
Review request changed

Status: Closed (submitted)

WJ
  1. This --guess flag behavior is one facet of the larger issue of config inheritance from ~/.reviewboardrc, $REPO/.reviewboardrc down through any command line options supplied. At least elsewhere, a common way to handle this is set the cli flag defaults to the values inhereited from the config file.

    https://stackoverflow.com/questions/3609852

    To me, it seems you've replicated this behavior except using a tristate enum. This pattern gets a bit wierder when extended to non boolean values. Consider the inheritiance of the --repository option. All of a sudden you're taking arbitray string values except 'auto' is special cased, and a repository can't be named 'auto' without breaking things (far-fetched... but still).

  2. 
      
Loading...