flake8
failed.
JSHint
passed.
Add cargo tool
Review Request #10337 — Created Nov. 29, 2018 and discarded
This adds a Review Bot tool for analyzing Rust code using the cargo subcommands
Clippy (for linting Rust code) and rustfmt (for checking code formatting
against style guidelines). The cargo tool includes options for which Clippy
linters to include or exclude.
The cargo tool has been tested manually on a Rust project. The Clippy options
have also been tested.
| Description | From | Last Updated |
|---|---|---|
|
F401 'reviewbot.utils.filesystem.chdir' imported but unused |
 reviewbot
|
|
|
E302 expected 2 blank lines, found 1 |
reviewbot
|
|
|
F821 undefined name 'command' |
reviewbot
|
|
|
F821 undefined name 'command' |
reviewbot
|
|
|
F821 undefined name 'command' |
reviewbot
|
|
|
F841 local variable 'path' is assigned to but never used |
reviewbot
|
|
|
F841 local variable 'working_dir' is assigned to but never used |
reviewbot
|
|
|
W293 blank line contains whitespace |
reviewbot
|
|
|
W391 blank line at end of file |
reviewbot
|
|
|
F401 'os' imported but unused |
reviewbot
|
|
|
Do we want to add an option to specify a toolchain (e.g., stable vs nightly or beta)? You can specify … |
 brennie
|
|
|
E128 continuation line under-indented for visual indent |
reviewbot
|
|
|
No blank line here. |
brennie
|
|
|
We should set manifest_dir to None unless we find it. If we don't find it, we should throw an error. |
brennie
|
|
|
Missing a period. |
brennie
|
|
|
A project can have multiple Cargo.toml files if it is set up as a workspace. In this case, we only … |
brennie
|
|
|
We should use os.path.basedir instead of unicode.replace to determine the manifest directory. |
brennie
|
|
|
No blank line here. |
brennie
|
|
|
Whatever path manipulation you are doing should be done with os.path. Does this strip the leading directory? See my other … |
brennie
|
|
|
Comments should end with a period. |
brennie
|
|
|
Blank line between these. |
brennie
|
|
|
Comments should end with a period. |
brennie
|
|
|
E501 line too long (80 > 79 characters) |
reviewbot
|
|
|
See my other comment about joining output. |
brennie
|
|
|
E303 too many blank lines (2) |
reviewbot
|
|
|
No blank line here. |
brennie
|
|
|
E126 continuation line over-indented for hanging indent |
reviewbot
|
|
|
Can we rename e to setting ? |
brennie
|
|
|
Can we rename d to setting ? |
brennie
|
|
|
INstead of doing addition, can we save this in a list on the class and then set self.output at the … |
brennie
|
|
|
Blank line between these. |
brennie
|
|
|
E501 line too long (82 > 79 characters) |
reviewbot
|
|
|
E722 do not use bare except' |
reviewbot
|
|
|
We never want to use a bare except: clause, as this will catch things we don't intend, like RuntimeError, SystemError, … |
brennie
|
|
|
"cargo support", since cargo doesn't actually require those things. |
brennie
|
|
|
We should link to rustup.rs |
brennie
|
|
|
E126 continuation line over-indented for hanging indent |
reviewbot
|
- Summary:
-
[WIP] Adding Cargo ToolAdd cargo tool
- Description:
-
~ Adding Cargo Tool
~ This adds a Review Bot tool for analyzing Rust code using the cargo subcommands
+ Clippy (for linting Rust code) and rustfmt (for checking code formatting + against style guidelines). The cargo tool includes options for which Clippy + linters to include or exclude. - Testing Done:
-
+ The cargo tool has been tested manually on a Rust project. The Clippy options
+ have also been tested. - Commit:
-
deed5636b135c6f43ddbc2106fcb478ba984a7fa59023f28160486ea2fe7dcdeb11b18246ef49efc
Checks run (1 failed, 1 succeeded)
flake8
failed.
JSHint
passed.
flake8
-
-
If you have time, adding support for
cargo checkwould be great! It does pre-compilation type and borrow checking. -
Do we want to add an option to specify a toolchain (e.g.,
stablevsnightlyorbeta)?You can specify the toolchain to commands like so:
cargo +nightly clippy #... rustfmt +beta #...For testing you can add additional toolchains with
rustup toolchain install beta -
-
We should set
manifest_dirtoNoneunless we find it.If we don't find it, we should throw an error.
-
-
A project can have multiple
Cargo.tomlfiles if it is set up as a workspace. In this case, we only want to runcargo clippycommands once for the top-level manifest directory. -
-
-
Whatever path manipulation you are doing should be done with
os.path. Does this strip the leading directory?See my other comment about having multiple
Cargo.tomlper project. -
-
-
-
-
-
-
-
INstead of doing addition, can we save this in a list on the class and then set
self.outputat the end ofhandle_files? e.g.def handle_files(self, files, settings): self.output_parts = [] # Actually handle files self.output = '\n'.join(self.output_parts) def run_cargo_clippy(self, f, settings): # ... self.output_parts.append( # ... )I suggest this becuase doing repeated string addition is slow and requires a bunch of successively larger allocations, instead of small allocations (to hold each part) and one large allocation to hold the entire result.
-
-
We never want to use a bare
except:clause, as this will catch things we don't intend, likeRuntimeError,SystemError,SystemExit, or evenKeyboardInterrupt(ctrl+c).None of these special errors inherit from
Exceptionso instead we can doexcept Exception:which lets those special errors happen as intended and we still catch exceptions we mean to.However, what exceptions are we catching here? Can we narrow it down to 1 or 2 specific exceptions instead of catching the entire world?
-
-
