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 |
- Commit:
-
bd8896f5d8bf78f2e2bbdef718c6a76c0e4f5b2ef30dfb0c85855978bd2e6e1d32294d970855daca
Checks run (2 succeeded)
-
-
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. -
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. -
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. -
-
The
int(line_num)
will crash ifline_num
isn't what we expect, so it should go within anytry/except
. -
-
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:
-
Cargo Tool for ReviewBotCargo Tool for Review Bot
- Commit:
-
f30dfb0c85855978bd2e6e1d32294d970855daca32c7ede11355ecbacdacd6edd93c54ddef6181cb
Checks run (2 succeeded)
- Change Summary:
-
Addressed David's comments and updated docstring.
- Commit:
-
32c7ede11355ecbacdacd6edd93c54ddef6181cb041f5346e0b765283552157a851a6ff9137262c6
Checks run (2 succeeded)
- Change Summary:
-
Fixed docstring.
- Commit:
-
041f5346e0b765283552157a851a6ff9137262c6f97dab08aa7f0985dd13a0e1b85cc020f794359d