Quality of life fix for rbt post when updating request

Review Request #9442 — Created Dec. 15, 2017 and submitted

Information

RBTools
master
24cc00f...

Reviewers

The issue: you post a review request with a different set of target
people or groups than are in your .reviewboardrc. When you then update
this existing review request, if you don't re-specify your custom
people/groups, then rbt will overwrite your custom choices with the ones
specified in your .reviewboardrc.

With this change, rbt post will use the options in the .reviewboardrc
only when creating a new review request. If you are updating an existing
request, then it will ignore the settings in the .reviewboardrc. You can
still manually change the groups/people in your update if you would
like. Unit tests show this functionality.

Been using this at work successfully.
Manually walked through all the posting options.
Unit tests should cover each case.

Description From Last Updated

Something went a little weird when posting your latest revision. It looks like you ended up posting the changes only …

daviddavid

Should explicitly say "review request" instead of just "request"

daviddavid

The loaded config should already exist as self.config

daviddavid

Add a blank line between these two.

daviddavid

When we do multi-line conditionals, we prefer parens rather than the continuation character: if (self.options.target_groups is None and 'TARGET_GROUPS' in …

daviddavid

Add a blank line between these two.

daviddavid

Parens here too.

daviddavid

This can just be '.reviewboardrc' instead of './.reviewboardrc' That said, do we need to make sure that this is running …

daviddavid

In order to avoid leaving the temporary file around in case the test case fails unexpectedly, this should probably be: …

daviddavid

Should include the type: args (list):

daviddavid

Not all arguments have values. I'd just say that the command line will include each of the items within the …

daviddavid

Each of your new test cases needs a docstring in order to print a description when running the test suite. …

daviddavid

Add a blank line between these two.

daviddavid

Add a blank line between these two.

daviddavid

Add a blank line between these two.

daviddavid

E501 line too long (83 > 79 characters)

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

We don't compare booleans against True or False. Instead, this should just do if not self.options.update

chipx86chipx86

Private methods should go at the end of the class. That said, I think this is better as a public …

chipx86chipx86

This can be hard-coded in the open call.

chipx86chipx86

The issue with a chdir is that any command being tested will no longer have access to the source code …

chipx86chipx86

This would be better as: with open(test_file, 'w') as fp: for ...: ...

chipx86chipx86

The file should import six, and then this would be: for key, value in six.iteritems(data): ...

chipx86chipx86

A couple important things to note here: This is assuming only strings are passed as values, which isn't always going …

chipx86chipx86

Is there a reason to flush above and then only close now? I'd suspect we'd want to just close the …

chipx86chipx86

This isn't needed. The rmdir will be sufficient.

chipx86chipx86

This will fail if anything else is written to the directory. We should use shutils.rmtree, and only do this if …

chipx86chipx86

Should be list of unicode.

chipx86chipx86

Docstrings for unit tests shouldn't end in a period. Here and below.

chipx86chipx86

Instead of just having True, can we do use_temp_dir=True?

daviddavid

Same here.

daviddavid

Same here.

daviddavid

Same here.

daviddavid

It was like this before, but this return type/description is incorrect. It returns the command instance.

daviddavid
david
  1. This is a great change! I have a bunch of comments, but they're mostly just style nits.

  2. rbtools/commands/post.py (Diff revision 1)
     
     
    Show all issues

    Should explicitly say "review request" instead of just "request"

  3. rbtools/commands/post.py (Diff revision 1)
     
     
    Show all issues

    The loaded config should already exist as self.config

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

    Add a blank line between these two.

  5. rbtools/commands/post.py (Diff revision 1)
     
     
     
    Show all issues

    When we do multi-line conditionals, we prefer parens rather than the continuation character:

    if (self.options.target_groups is None and
        'TARGET_GROUPS' in config):
        ....
    
  6. rbtools/commands/post.py (Diff revision 1)
     
     
     
    Show all issues

    Add a blank line between these two.

  7. rbtools/commands/post.py (Diff revision 1)
     
     
     
    Show all issues

    Parens here too.

  8. rbtools/commands/tests/test_post.py (Diff revision 1)
     
     
    Show all issues

    This can just be '.reviewboardrc' instead of './.reviewboardrc'

    That said, do we need to make sure that this is running within a temporary directory? What is the cwd when these tests run?

    1. cwd seems to depend on what directory you run nosetest from. Right now I set it to be in the rbtools/commands folder, as it doesn't appear that load_config will pick it up in the tests dir. Not sure if this is the best thing to do.

    2. I think what we probably should do is create a temporary directory, chdir to that, and then write the file.

  9. rbtools/commands/tests/test_post.py (Diff revision 1)
     
     
     
     
     
    Show all issues

    In order to avoid leaving the temporary file around in case the test case fails unexpectedly, this should probably be:

    try:
        yield
    finally:
        tmp.close()
        os.unlink('.reviewboardrc')
    
  10. rbtools/commands/tests/test_post.py (Diff revision 1)
     
     
    Show all issues

    Should include the type:

    args (list):
    
  11. rbtools/commands/tests/test_post.py (Diff revision 1)
     
     
     
    Show all issues

    Not all arguments have values. I'd just say that the command line will include each of the items within the list.

  12. rbtools/commands/tests/test_post.py (Diff revision 1)
     
     
    Show all issues

    Each of your new test cases needs a docstring in order to print a description when running the test suite. See the above tests for examples of what they should look like.

  13. rbtools/commands/tests/test_post.py (Diff revision 1)
     
     
     
    Show all issues

    Add a blank line between these two.

  14. rbtools/commands/tests/test_post.py (Diff revision 1)
     
     
     
    Show all issues

    Add a blank line between these two.

  15. rbtools/commands/tests/test_post.py (Diff revision 1)
     
     
     
    Show all issues

    Add a blank line between these two.

  16. 
      
RW
Review request changed
Commit:
db6a75385b407c1ce90bc9f745df231df29373c4
4714e99123bf280784c54e947af9be3ecfb91d27

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

chipx86
  1. Concept behind the change certainly seems reasonable.

    I wanted to touch upon the Notion issue. It should be public. What link are you trying? I want to figure out if something regressed.

    1. the README links to https://www.reviewboard.org/docs/codebase/dev/ which links to https://www.notion.so/reviewboard/Review-Board-8nXunbyOuorZ2 .

    2. Looks like they changed how this works. Does this link work better for you? https://www.notion.so/reviewboard/Review-Board-45d228fb07a0459b84fee509ac054cec

      I'll get the links on the site updated.

    3. Yeah, that one works!

  2. 
      
RW
RW
david
  1. 
      
  2. Show all issues

    Something went a little weird when posting your latest revision. It looks like you ended up posting the changes only in the latest commit and not the full diff between master and your topic branch (probably via something like rbt post HEAD). This breaks the interdiffs and doesn't let us use rbt land to land the change. Would you mind re-posting by running rbt post -r 9442 master..mybranch?

    1. Hm thats odd. I was just using rbt post -u I think. I re-posted using the command you gave.

    2. Hmm. Things still look weird. Are your origin/master and master HEADs pointing to the upstream master commit?

    3. Ah, I'd apparently pushed my first commit to my forked master. Didn't realized I'd done that. Lets see if this one works better.

    4. Looks good now, thanks!

  3. 
      
RW
RW
chipx86
  1. 
      
  2. rbtools/commands/post.py (Diff revision 6)
     
     
    Show all issues

    We don't compare booleans against True or False. Instead, this should just do if not self.options.update

  3. rbtools/commands/tests/test_post.py (Diff revision 6)
     
     
     
    Show all issues

    Private methods should go at the end of the class. That said, I think this is better as a public function in RBTestBase, so that other commands can benefit. I'd call this just reviewboardrc.

    This actually should apply to _create_post_command as well, even though it wasn't following this rule before.

    This also needs a docstring.

  4. rbtools/commands/tests/test_post.py (Diff revision 6)
     
     
    Show all issues

    This can be hard-coded in the open call.

  5. rbtools/commands/tests/test_post.py (Diff revision 6)
     
     
    Show all issues

    The issue with a chdir is that any command being tested will no longer have access to the source code repository, if it's testing against one. I'd make the chdir optional. If the unit test is cloning a repository and doing a test in there, it's fine for it to write to that same directory.

  6. rbtools/commands/tests/test_post.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    This would be better as:

    with open(test_file, 'w') as fp:
        for ...:
            ...
    
  7. rbtools/commands/tests/test_post.py (Diff revision 6)
     
     
    Show all issues

    The file should import six, and then this would be:

    for key, value in six.iteritems(data):
        ...
    
  8. rbtools/commands/tests/test_post.py (Diff revision 6)
     
     
    Show all issues

    A couple important things to note here:

    1. This is assuming only strings are passed as values, which isn't always going to be true. Sometimes it will be booleans or lists.
    2. It also needs a newline, or multiple values are going to create an invalid file.

    Therefore, I'd switch this to:

    fp.write('%s = %r\n' % (key, value))
    

    The %r will generally return something suitable for the kinds of values we're working with. Down the road, we'll probably need to have a better serializer, but this is fine for now.

    (We generally use printf-style strings anyway.)

  9. rbtools/commands/tests/test_post.py (Diff revision 6)
     
     
    Show all issues

    Is there a reason to flush above and then only close now? I'd suspect we'd want to just close the file above (which my with suggestion will do).

  10. rbtools/commands/tests/test_post.py (Diff revision 6)
     
     
    Show all issues

    This isn't needed. The rmdir will be sufficient.

  11. rbtools/commands/tests/test_post.py (Diff revision 6)
     
     
    Show all issues

    This will fail if anything else is written to the directory. We should use shutils.rmtree, and only do this if that optional parameter I mentioned is provided.

  12. rbtools/commands/tests/test_post.py (Diff revision 6)
     
     
    Show all issues

    Should be list of unicode.

  13. rbtools/commands/tests/test_post.py (Diff revision 6)
     
     
    Show all issues

    Docstrings for unit tests shouldn't end in a period.

    Here and below.

  14. 
      
RW
david
  1. 
      
  2. rbtools/commands/tests/test_post.py (Diff revision 7)
     
     
    Show all issues

    Instead of just having True, can we do use_temp_dir=True?

  3. rbtools/commands/tests/test_post.py (Diff revision 7)
     
     
    Show all issues

    Same here.

  4. rbtools/commands/tests/test_post.py (Diff revision 7)
     
     
    Show all issues

    Same here.

  5. rbtools/commands/tests/test_post.py (Diff revision 7)
     
     
    Show all issues

    Same here.

  6. 
      
RW
david
  1. Just one last nit from me:

  2. rbtools/commands/tests/test_post.py (Diff revision 8)
     
     
     
    Show all issues

    It was like this before, but this return type/description is incorrect. It returns the command instance.

  3. 
      
RW
david
  1. Ship It!
  2. 
      
RW
Review request changed
Status:
Completed
Change Summary:
Pushed to master (40f55b3)