Cargo Tool for Review Bot
Review Request #11307 — Created Nov. 25, 2020 and submitted
The Cargo Tool utilizes,
cargo clippy
andcargo 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 thatcargo 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 |
ruonanjia | |
I think this can be one line. |
hailan | |
I am actually using the path variable later on in the loop on line 93, to access the correct dictionary … |
jblazusi | |
There should be an empty line before this. |
ceciliawei | |
switch with checkstyle for alphabetical order |
ruonanjia | |
This should have a try/except to catch any issues executing the program. Also, how about just calling this lines? I … |
chipx86 | |
Rather than range, we should use enumerate: num_lines = len(clippy_output) for i, line in enumerate(clippy_output): if i + 1 >= … |
chipx86 | |
Let's use filename instead of file_. It communicates it better and avoids the conflict. Also, we should never override _, … |
chipx86 | |
Can you update this to use single quotes, and alphabetize keys? |
chipx86 | |
The int(line_num) will crash if line_num isn't what we expect, so it should go within any try/except. |
chipx86 | |
This should have a try/except. |
chipx86 | |
Some of the same comments as above: enumerate (saves a lookup each iteration) Don't assume a split is successful Don't … |
chipx86 | |
No need for parens around test_name. |
chipx86 | |
The official terminology seems to be "Cargo commands" |
david | |
I checked the code base and looks like the style for logger.exception is logger.exception('some string: %s',e). Maybe change the % … |
ceciliawei |
-
-
bot/reviewbot/tools/cargotool.py (Diff revision 1) 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!
-
-
bot/reviewbot/tools/cargotool.py (Diff revision 1) wrap with parenthesis instead of line break character to be more consistent with rest of codebase
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+108) |
Checks run (2 succeeded)
-
-
bot/reviewbot/tools/cargotool.py (Diff revision 2) 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. -
bot/reviewbot/tools/cargotool.py (Diff revision 2) Rather than
range
, we should useenumerate
: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 andlstrip
it again inside the block. -
bot/reviewbot/tools/cargotool.py (Diff revision 2) Let's use
filename
instead offile_
. 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 forugettext
/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. -
bot/reviewbot/tools/cargotool.py (Diff revision 2) Can you update this to use single quotes, and alphabetize keys?
-
bot/reviewbot/tools/cargotool.py (Diff revision 2) The
int(line_num)
will crash ifline_num
isn't what we expect, so it should go within anytry/except
. -
-
bot/reviewbot/tools/cargotool.py (Diff revision 2) Some of the same comments as above:
enumerate
(saves a lookup each iteration)- Don't assume a split is successful
- Don't override
_
-
Change Summary:
Addressed Christian's comments and applied his recommendations.
Summary: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 3 (+125) |
Checks run (2 succeeded)
-
-
bot/reviewbot/tools/cargotool.py (Diff revision 3) The official terminology seems to be "Cargo commands"
Change Summary:
Addressed David's comments and updated docstring.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+125) |
Checks run (2 succeeded)
Change Summary:
Fixed docstring.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+125) |
Checks run (2 succeeded)
-
-
bot/reviewbot/tools/cargotool.py (Diff revision 5) I checked the code base and looks like the style for
logger.exception
islogger.exception('some string: %s',e)
. Maybe change the%
to a comma to make it consistent?
Change Summary:
Fixed
logger.exception
formatting.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+125) |