Cargo Tool for Review Bot

Review Request #11307 — Created Nov. 25, 2020 and submitted

Information

ReviewBot
master
45a5213...

Reviewers

The Cargo Tool utilizes, cargo clippy and cargo test to lint and
test Rust code for errors that could be potentially expensive. To do
this, the Cargo Tool requires access to the entire repository so that
it can succesfully build the project with the patched code.
This is necessary so that a project can be accurately analyzed.

Manual testing was done to confirm that cargo test is working
correctly. As of now it just creates general comments based on which
tests have failed. It is worth noting, that if the files do not compile
correctly, no comments will be made.
Manual testing was done to confirm that cargo clippy creates comments
only for patched files.


Description From Last Updated

wrap with parenthesis instead of line break character to be more consistent with rest of codebase

ruonanjiaruonanjia

I think this can be one line.

hailanhailan

I am actually using the path variable later on in the loop on line 93, to access the correct dictionary …

jblazusijblazusi

There should be an empty line before this.

ceciliaweiceciliawei

switch with checkstyle for alphabetical order

ruonanjiaruonanjia

This should have a try/except to catch any issues executing the program. Also, how about just calling this lines? I …

chipx86chipx86

Rather than range, we should use enumerate: num_lines = len(clippy_output) for i, line in enumerate(clippy_output): if i + 1 >= …

chipx86chipx86

Let's use filename instead of file_. It communicates it better and avoids the conflict. Also, we should never override _, …

chipx86chipx86

Can you update this to use single quotes, and alphabetize keys?

chipx86chipx86

The int(line_num) will crash if line_num isn't what we expect, so it should go within any try/except.

chipx86chipx86

This should have a try/except.

chipx86chipx86

Some of the same comments as above: enumerate (saves a lookup each iteration) Don't assume a split is successful Don't …

chipx86chipx86

No need for parens around test_name.

chipx86chipx86

The official terminology seems to be "Cargo commands"

daviddavid

I checked the code base and looks like the style for logger.exception is logger.exception('some string: %s',e). Maybe change the % …

ceciliaweiceciliawei
jblazusi
  1. 
      
  2. 
      
jblazusi
hailan
  1. 
      
  2. bot/reviewbot/tools/cargotool.py (Diff revision 1)
     
     
     
    Show all issues

    I think this can be one line.

  3. 
      
jblazusi
  1. 
      
  2. bot/reviewbot/tools/cargotool.py (Diff revision 1)
     
     
     
    Show all issues

    I am actually using the path variable later on in the loop on line 93, to access the correct dictionary key. Unfortunately, I do not think I can make this one line without using a ternary operator, which is best avoided.

    Thank you for the review!

    1. Ah?that makes sense. Sorry, I missed that.

  3. 
      
ceciliawei
  1. 
      
  2. bot/reviewbot/tools/cargotool.py (Diff revision 1)
     
     
    Show all issues

    There should be an empty line before this.

    1. Fixed, Thank you for the review!

  3. 
      
ruonanjia
  1. 
      
  2. bot/setup.py (Diff revision 1)
     
     
    Show all issues

    switch with checkstyle for alphabetical order

  3. 
      
ruonanjia
  1. 
      
  2. bot/reviewbot/tools/cargotool.py (Diff revision 1)
     
     
     
    Show all issues

    wrap with parenthesis instead of line break character to be more consistent with rest of codebase

    1. Good catch, Thank you for the review!

  3. 
      
jblazusi
chipx86
  1. 
      
  2. bot/reviewbot/tools/cargotool.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    This should have a try/except to catch any issues executing the program.

    Also, how about just calling this lines? I think it communicates things better.

  3. bot/reviewbot/tools/cargotool.py (Diff revision 2)
     
     
    Show all issues

    Rather than range, we should use enumerate:

    num_lines = len(clippy_output)
    
    for i, line in enumerate(clippy_output):
        if i + 1 >= num_lines:
            break
    
        next_line = clippy_output[i + 1].lstrip()
    
        if (line.startswith(prefixes) and
            next_line.startswith('--> ')):
            ...
    

    Note the addition of the first check in that loop. As-is, your code could end up crashing at a boundary.

    Also, pulling out next_line, so we don't have to pull it out and lstrip it again inside the block.

  4. bot/reviewbot/tools/cargotool.py (Diff revision 2)
     
     
    Show all issues

    Let's use filename instead of file_. It communicates it better and avoids the conflict.

    Also, we should never override _, as that's used in our code (or really anything using gettext localization) as an alias for ugettext/ugettext_lazy, and will inevitably result in that being overridden.

    One final note: We should have a try/except here, because if we get a string here that has too many or too few items, we'll crash.

  5. bot/reviewbot/tools/cargotool.py (Diff revision 2)
     
     
     
    Show all issues

    Can you update this to use single quotes, and alphabetize keys?

  6. bot/reviewbot/tools/cargotool.py (Diff revision 2)
     
     
    Show all issues

    The int(line_num) will crash if line_num isn't what we expect, so it should go within any try/except.

  7. bot/reviewbot/tools/cargotool.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    This should have a try/except.

  8. bot/reviewbot/tools/cargotool.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    Some of the same comments as above:

    • enumerate (saves a lookup each iteration)
    • Don't assume a split is successful
    • Don't override _
  9. bot/reviewbot/tools/cargotool.py (Diff revision 2)
     
     
    Show all issues

    No need for parens around test_name.

  10. 
      
jblazusi
david
  1. 
      
  2. bot/reviewbot/tools/cargotool.py (Diff revision 3)
     
     
    Show all issues

    The official terminology seems to be "Cargo commands"

  3. 
      
jblazusi
jblazusi
ceciliawei
  1. 
      
  2. bot/reviewbot/tools/cargotool.py (Diff revision 5)
     
     
    Show all issues

    I checked the code base and looks like the style for logger.exception is logger.exception('some string: %s',e). Maybe change the % to a comma to make it consistent?

    1. That is interesting, I have never noticed that before. Is there a reason why a comma is used instead of % specifically for logger.exception? Maybe the mentors could explain?

    2. I think I found the reason. https://docs.python.org/3/library/logging.html#logging.Logger.debug
      What you were doing is to merge the exception into the string cargo clippy failed: %s' before passing it to the logger. According to the documentation for Logger, logger.exception/debug/... takes in a msg and multiple (optional) args, ...and the args are the arguments which are merged into msg using the string formatting operator. (Note that this means that you can use keywords in the format string, together with a single dictionary argument.) No % formatting operation is performed on msg when no args are supplied.
      I think the other places are following the documentation for this method?

    3. Perfect! That makes sense, thank you for this clarification. ????

  3. 
      
jblazusi
david
  1. Ship It!
  2. 
      
jblazusi
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.0.x (051d3dc)