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...