Add the `rbt review` command.

Review Request #11883 — Created Nov. 24, 2021 and submitted

david
RBTools
master
11882
rbtools

This change adds a new RBTools command for creating and editing reviews.
This uses the new MultiCommand subclass in order to provide a variety
of functionality within a single command:

  • rbt review edit will create or edit a draft review.
  • rbt review discard will discard a draft review.
  • rbt review publish will publish a draft review.
  • rbt review add-diff-comment will add a new diff comment.
  • rbt review add-file-attachment-comment will add a new file
    attachment comment.
  • rbt review add-general-comment will add a new general comment.

Based on work by Anahita Mohapatra at /r/11502/.

Ran through manual test cases for each of the review subcommands:
- Creating a new review with various content.
- Editing an existing draft review.
- Creating general comments (both markdown and plain)
- Creating file attachment comments (both markdown and plain)
- Creating diff comments (markdown and plain, single- and multi-line).
- Discarding a draft review.
- Publishing a draft review.
- Tested validation and error handling (file attachment IDs, diff
filenames, diff revisions)

Summary
Add the `rbt review` command.
Description From Last Updated

F401 'argparse' imported but unused

reviewbotreviewbot

F401 'sys' imported but unused

reviewbotreviewbot

F401 'six' imported but unused

reviewbotreviewbot

F841 local variable 'file_attachment' is assigned to but never used

reviewbotreviewbot

F841 local variable 'e' is assigned to but never used

reviewbotreviewbot

rbt review <subcommand> --help?

chipx86chipx86

Can you add a Version Added?

chipx86chipx86

Can we give the base classes a Base prefix?

chipx86chipx86

Some of the commands do some lookups and possible CommandError raising before operating on a draft. If we do this ...

chipx86chipx86

We shouldn't need to provide this. It'll be the default (and will use the correct string type).

chipx86chipx86

Typo: MARKDOMN -> MARKDOWN

chipx86chipx86

Can we end all the help_text strings with a period, like we do in Command.description and help= strings? Same applies ...

chipx86chipx86

We use uppercase metavars in all options in the codebase. Can you update all the ones in the change to ...

chipx86chipx86

This isn't needed.

chipx86chipx86

This should probably use default=1. I think we can then avoid checking against None below.

chipx86chipx86

Typo: "specifieud"

chipx86chipx86

We do self.options all over the place. Let's pull it out into a local variable, reduce the noise and the ...

chipx86chipx86

"fid" is kind of confusing. Can we just make it "ID"?

chipx86chipx86

"id" -> "ID" I don't think we need the quotes around the ID numbers. We don't use them elsewhere.

chipx86chipx86

Missing a trailing period.

chipx86chipx86

Typo: MARKDOMN -> MARKDOWN. I state in another comment that we may want to consider a standard option we can ...

chipx86chipx86

I was looking into whether we could avoid having to use two variables and the complex logic below but also ...

chipx86chipx86

Missing a trailing period.

chipx86chipx86

We have this logic three times in two existing commands, and are adding more. While --markdown isn't a standard argument ...

chipx86chipx86

The trailing newline isn't needed here.

chipx86chipx86

Missing a trailing period.

chipx86chipx86

Can we call this create_review_if_missing, and document it?

chipx86chipx86

This can use the new helper function, right?

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. docs/rbtools/rbt/commands/review.rst (Diff revision 2)
     
     

    rbt review <subcommand> --help?

  3. rbtools/commands/review.py (Diff revision 2)
     
     

    Can you add a Version Added?

  4. rbtools/commands/review.py (Diff revision 2)
     
     

    Can we give the base classes a Base prefix?

  5. rbtools/commands/review.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Some of the commands do some lookups and possible CommandError raising before operating on a draft. If we do this first, we can easily end up with empty drafts hanging around.

    How about just moving this to a get_review_draft method or something, and calling it when ready to work on the draft?

  6. rbtools/commands/review.py (Diff revision 2)
     
     

    We shouldn't need to provide this. It'll be the default (and will use the correct string type).

  7. rbtools/commands/review.py (Diff revision 2)
     
     

    Typo: MARKDOMN -> MARKDOWN

  8. rbtools/commands/review.py (Diff revision 2)
     
     

    Can we end all the help_text strings with a period, like we do in Command.description and help= strings?

    Same applies below.

  9. rbtools/commands/review.py (Diff revision 2)
     
     

    We use uppercase metavars in all options in the codebase. Can you update all the ones in the change to be uppercase?

    (Should also be one word: "FILENAME")

  10. rbtools/commands/review.py (Diff revision 2)
     
     

    This isn't needed.

  11. rbtools/commands/review.py (Diff revision 2)
     
     
     
     
     
     

    This should probably use default=1. I think we can then avoid checking against None below.

  12. rbtools/commands/review.py (Diff revision 2)
     
     

    Typo: "specifieud"

  13. rbtools/commands/review.py (Diff revision 2)
     
     

    We do self.options all over the place. Let's pull it out into a local variable, reduce the noise and the repeated attribute lookups.

    Same with api_root.

    This applies to other implementations as well.

    1. api_root is used once, maybe twice per command. It doesn't seem worth it for that. Did it for options.

  14. rbtools/commands/review.py (Diff revision 2)
     
     

    "fid" is kind of confusing. Can we just make it "ID"?

  15. rbtools/commands/review.py (Diff revision 2)
     
     
     

    "id" -> "ID"

    I don't think we need the quotes around the ID numbers. We don't use them elsewhere.

  16. rbtools/commands/review.py (Diff revision 2)
     
     

    Missing a trailing period.

  17. rbtools/commands/review.py (Diff revision 2)
     
     

    Typo: MARKDOMN -> MARKDOWN.

    I state in another comment that we may want to consider a standard option we can plug in for Markdown, and given the error here, maybe this is a good time to do it?

  18. rbtools/commands/review.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     

    I was looking into whether we could avoid having to use two variables and the complex logic below but also gain the useful error message about mutual-exclusivity.

    argparse has an add_mutually_exclusive_group for this purpose, but it's probably too heavy-weight, and we don't have an Option/OptionGroup equivalent for it (though we could add it).

    Anyway, so I checked other commands, and this scenario is all over the place, and probably isn't a big deal. A --thing and --no-thing both populating the same variable is common (such as rbt land --push and --no-push), and we haven't heard of any confusion around them yet.

    So I think it'd be better to err on the side of simplicity here, have both these options use the same variable and default=None (which is the default value for default anyway), and then trust the value when making decisions below.

    If we want to add mutual-exclusive support during parsing, we should probably handle that in common code somehow.

  19. rbtools/commands/review.py (Diff revision 2)
     
     

    Missing a trailing period.

  20. rbtools/commands/review.py (Diff revision 2)
     
     
     
     
     
     

    We have this logic three times in two existing commands, and are adding more.

    While --markdown isn't a standard argument (we should probably make a constant for it though), I think it'd be valuable to have at least a utility function that gives us a text type for a given value + capability, and just use that.

    1. I don't know that we can have a good constant for it because the help text is different in every instance. I'll add the helper though.

  21. rbtools/commands/review.py (Diff revision 2)
     
     

    The trailing newline isn't needed here.

  22. rbtools/commands/review.py (Diff revision 2)
     
     

    Missing a trailing period.

  23. 
      
david
chipx86
  1. 
      
  2. rbtools/commands/review.py (Diff revision 3)
     
     

    Can we call this create_review_if_missing, and document it?

  3. rbtools/commands/review.py (Diff revision 3)
     
     
     
     
     
     

    This can use the new helper function, right?

  4. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (7c746df)
Loading...