Quality of life fix for rbt post when updating request

Review Request #9442 - Created Dec. 15, 2017 and updated

Ryan Wooster
RBTools
master
20ee4de...
rbtools

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.

  • 0
  • 0
  • 29
  • 0
  • 29
Description From Last Updated
David Trowbridge
  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)
     
     

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

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

    The loaded config should already exist as self.config

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

    Add a blank line between these two.

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

    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)
     
     
     

    Add a blank line between these two.

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

    Parens here too.

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

    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)
     
     
     
     
     

    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)
     
     

    Should include the type:

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

    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)
     
     

    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)
     
     
     

    Add a blank line between these two.

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

    Add a blank line between these two.

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

    Add a blank line between these two.

  16. 
      
Ryan Wooster
Review request changed

Commit:

-db6a75385b407c1ce90bc9f745df231df29373c4
+4714e99123bf280784c54e947af9be3ecfb91d27

Diff:

Revision 2 (+46 -22)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Christian Hammond
  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. 
      
Ryan Wooster
Ryan Wooster
David Trowbridge
  1. 
      
  2. 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. 
      
Ryan Wooster
Ryan Wooster
Christian Hammond
  1. 
      
  2. rbtools/commands/post.py (Diff revision 6)
     
     

    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)
     
     
     

    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)
     
     

    This can be hard-coded in the open call.

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

    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)
     
     
     
     
     

    This would be better as:

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

    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)
     
     

    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)
     
     

    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)
     
     

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

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

    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)
     
     

    Should be list of unicode.

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

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

    Here and below.

  14. 
      
Ryan Wooster
Review request changed

Change Summary:

Updated with new review comments.

Description:

-  

I can't actually access the contributor guide so apologies if I'm missing something. Apparently I need to be granted access to the Notion Wiki? But it doesn't give me anyone to contact.

-  
-  

-  
   

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.

Commit:

-8cd45eb7105db136116b70f4f189729d1ea98085
+20ee4deedd2b4dbd6f82e3a982a5cf92c59b6465

Diff:

Revision 7 (+152 -23)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...