Add status-update command to rbtools.

Review Request #9343 — Created Nov. 1, 2017 and submitted

bleblan2
RBTools
release-0.7.x
dd36fbb...
rbtools, students

Review Board 3.0 introduced new support for "status updates" which are
used to indicate if a review request is undergoing some automated checks
of some kind (for instance, automated code review or pending builds).
These can have a state (such as "pending", "error", "success", and
others), a summary, optional link, and a review (when there are errors).

This adds a new command for working with status updates, helping to
create/update the state of a status update. This command can be run with
rbt status-update.

Debug testing of the --review input on local installation with the following file contents:

{
    "review": {
        "body_top": "Header comment"
    },
    "diff_comment": [
        {
            "filediff_id": 10,
            "first_line": 729,
            "issue_opened": true,
            "num_lines": 1,
            "text": "Adding a comment on a diff line",
            "text_type": "markdown"
        }
    ],
    "general_comments": [
        {
            "text": "Adding a general comment",
            "text_type": "markdown"
        }
    ]
}
  • 0
  • 0
  • 25
  • 2
  • 27
Description From Last Updated
brennie
  1. 
      
  2. rbtools/commands/status_update.py (Diff revision 1)
     
     

    You might want to pull out each action into its own function.

  3. 
      
bleblan2
bleblan2
bleblan2
bleblan2
david
  1. 
      
  2. rbtools/commands/status_update.py (Diff revision 5)
     
     

    I know most files in rbtools don't have them (yet), but we should have a file docstring at the top.

  3. rbtools/commands/status_update.py (Diff revision 5)
     
     
     

    Add a blank line between these two.

  4. rbtools/commands/status_update.py (Diff revision 5)
     
     

    Needs "Args" and "Returns" sections.

  5. rbtools/commands/status_update.py (Diff revision 5)
     
     
     
     

    We don't need to define a variable if we're immediately returning it. It would also be a bit nicer to wrap this as:

    return {
        key: status_update.get(key)
        for key in keys
        if status_update.get(key)
    }
    
  6. rbtools/commands/status_update.py (Diff revision 5)
     
     

    Needs an "Args" section.

  7. rbtools/commands/status_update.py (Diff revision 5)
     
     

    Needs "Args" and "Returns" sections.

  8. rbtools/commands/status_update.py (Diff revision 5)
     
     

    Needs "Args" and "Raises" sections.

  9. rbtools/commands/status_update.py (Diff revision 5)
     
     

    Needs "Args" and "Raises" sections.

  10. rbtools/commands/status_update.py (Diff revision 5)
     
     

    It's kind of confusing how this can either be an individual status update or the list of all status updates. I think I'd prefer if the conditional (do we have self.options.sid) is the top-level construct and then we either update or create as appropriate.

  11. rbtools/commands/status_update.py (Diff revision 5)
     
     

    Needs "Args" and "Raises" sections.

  12. rbtools/commands/status_update.py (Diff revision 5)
     
     

    Needs "Args" and "Raises" sections.

  13. rbtools/commands/status_update.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    In addition to specifying the details like this, perhaps we can show an example JSON blob?

  14. rbtools/commands/status_update.py (Diff revision 5)
     
     
     

    paren can go on the previous line.

  15. rbtools/commands/status_update.py (Diff revision 5)
     
     
     
     

    Instead of using the continuation character, let's wrap the whole thing in parens:

    if (not file_contents.get('reviews') and
        not file_contents.get('diff_comments') and
        not file_contents.get('general_comments')):
    

    You can also use hasattr instead of get, which is a bit more efficient.

  16. rbtools/commands/status_update.py (Diff revision 5)
     
     

    get -> hasattr

  17. rbtools/commands/status_update.py (Diff revision 5)
     
     

    get -> hasattr

  18. rbtools/commands/status_update.py (Diff revision 5)
     
     

    get -> hasattr

  19. rbtools/commands/status_update.py (Diff revision 5)
     
     

    Needs a period.

  20. rbtools/commands/status_update.py (Diff revision 5)
     
     

    Needs "Args" and "Raises" sections.

  21. rbtools/commands/status_update.py (Diff revision 5)
     
     

    Needs a period.

  22. rbtools/commands/status_update.py (Diff revision 5)
     
     

    I'm guessing you haven't yet tested this functionality because you'll need to associate the newly created review ID with the status update.

    It's also a good idea to do that association before publishing the review, so it doesn't show up (momentarily) as a regular review.

  23. rbtools/utils/filesystem.py (Diff revision 5)
     
     

    mode is also unicode

  24. rbtools/utils/filesystem.py (Diff revision 5)
     
     

    mode is also unicode, but we shouldn't need to have it as an arg at all because we'll always be reading the file. We can just pass in 'r' to open.

  25. rbtools/utils/filesystem.py (Diff revision 5)
     
     

    You can avoid having the entire get_file function by doing:

    return json.load(open(path, 'r'))
    

    We shouldn't need expandvars or expanduser because the user's shell should have done the globbing for us.

  26. 
      
  1. Code looks good.

  2. 
      
bleblan2
  1. What's left to do here to get this out of WIP?

    1. Being able to attach the review that gets created by the json file to the status-update, I'm about to push a diff with those changes.

  2. 
      
bleblan2
bleblan2
  1. 
      
  2. Missing docs rst file for new status-update command.

  3. 
      
bleblan2
Review request changed

Change Summary:

Add docs rst file for status-update and add APIError catching to review creation.

Commit:

-5d28a093765d80ed303bd112795b1b8378fd2733
+640828e7cd8cb61e016991b922cc6674b3fdce6f

Diff:

Revision 8 (+538)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

bleblan2
david
  1. Landing with minor changes. Thanks!

  2. 
      
bleblan2
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-1.0.x (115b9c9)
Loading...