Add cargo tool

Review Request #10337 — Created Nov. 29, 2018 and discarded

Information

ReviewBot
master

Reviewers

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

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

F821 undefined name 'command'

reviewbotreviewbot

F821 undefined name 'command'

reviewbotreviewbot

F821 undefined name 'command'

reviewbotreviewbot

F841 local variable 'path' is assigned to but never used

reviewbotreviewbot

F841 local variable 'working_dir' is assigned to but never used

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

F401 'os' imported but unused

reviewbotreviewbot

Do we want to add an option to specify a toolchain (e.g., stable vs nightly or beta)? You can specify …

brenniebrennie

E128 continuation line under-indented for visual indent

reviewbotreviewbot

No blank line here.

brenniebrennie

We should set manifest_dir to None unless we find it. If we don't find it, we should throw an error.

brenniebrennie

Missing a period.

brenniebrennie

A project can have multiple Cargo.toml files if it is set up as a workspace. In this case, we only …

brenniebrennie

We should use os.path.basedir instead of unicode.replace to determine the manifest directory.

brenniebrennie

No blank line here.

brenniebrennie

Whatever path manipulation you are doing should be done with os.path. Does this strip the leading directory? See my other …

brenniebrennie

Comments should end with a period.

brenniebrennie

Blank line between these.

brenniebrennie

Comments should end with a period.

brenniebrennie

E501 line too long (80 > 79 characters)

reviewbotreviewbot

See my other comment about joining output.

brenniebrennie

E303 too many blank lines (2)

reviewbotreviewbot

No blank line here.

brenniebrennie

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Can we rename e to setting ?

brenniebrennie

Can we rename d to setting ?

brenniebrennie

INstead of doing addition, can we save this in a list on the class and then set self.output at the …

brenniebrennie

Blank line between these.

brenniebrennie

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E722 do not use bare except'

reviewbotreviewbot

We never want to use a bare except: clause, as this will catch things we don't intend, like RuntimeError, SystemError, …

brenniebrennie

"cargo support", since cargo doesn't actually require those things.

brenniebrennie

We should link to rustup.rs

brenniebrennie

E126 continuation line over-indented for hanging indent

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

ilaw
Review request changed

Summary:

-[WIP] Adding Cargo Tool
+Add 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:

-deed5636b135c6f43ddbc2106fcb478ba984a7fa
+59023f28160486ea2fe7dcdeb11b18246ef49efc

Diff:

Revision 2 (+239)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ilaw
Review request changed

Commit:

-59023f28160486ea2fe7dcdeb11b18246ef49efc
+5ef2f194edb961e38ab5ff7af979ff39ca53a24f

Diff:

Revision 3 (+238)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
  1. 
      
  2. If you have time, adding support for cargo check would be great! It does pre-compilation type and borrow checking.

    1. I looked at implementing cargo check but it seems to check similar things as cargo clippy and returned the same results on my test files. I couldn't find much information on the difference between the two and seems like I would have to run cargo clean between them if I wanted to run both.

  3. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     

    Do we want to add an option to specify a toolchain (e.g., stable vs nightly or beta)?

    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

    1. I've added the options and installed the toolchains but there seems to be an issue with rustup and these subcommands so I've only tested the stable option for now.

  4. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     

    No blank line here.

  5. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     

    We should set manifest_dir to None unless we find it.

    If we don't find it, we should throw an error.

  6. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     

    Missing a period.

  7. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     
     
     
     

    A project can have multiple Cargo.toml files if it is set up as a workspace. In this case, we only want to run cargo clippy commands once for the top-level manifest directory.

    1. Is there a good way to check whether it is the top-level manifest directory? I was thinking we could scan all the files and return when it finds the first Cargo.toml file but that might take a long time for bigger repositories. Otherwise, we can compare the Cargo.toml files and return the one with the shortest path.

  8. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     

    We should use os.path.basedir instead of unicode.replace to determine the manifest directory.

  9. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     

    No blank line here.

  10. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     

    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.toml per project.

    1. Yes, I'm trying to strip the leading directory (where the Cargo.toml file is) because that is how the errors were saved after executing cargo clippy i.e. src/main.rs instead of testcargo/hello_world/src/main.rs with the Cargo.toml file being testcargo/hello_world/Cargo.toml. I went through the os.path module but didn't find a good way to do this besides splitting and then rejoining without the manifest directory.

  11. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     

    Comments should end with a period.

  12. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     
     

    Blank line between these.

  13. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     

    Comments should end with a period.

  14. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     
     
     
     
     

    See my other comment about joining output.

  15. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     

    No blank line here.

  16. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     

    Can we rename e to setting ?

  17. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     

    Can we rename d to setting ?

  18. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     
     
     
     

    INstead of doing addition, can we save this in a list on the class and then set self.output at the end of handle_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.

  19. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     
     

    Blank line between these.

  20. bot/reviewbot/tools/cargo.py (Diff revision 2)
     
     

    We never want to use a bare except: clause, as this will catch things we don't intend, like RuntimeError, SystemError, SystemExit, or even KeyboardInterrupt (ctrl+c).

    None of these special errors inherit from Exception so instead we can do except 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?

  21. docs/reviewbot/tools/cargo.rst (Diff revision 2)
     
     

    "cargo support", since cargo doesn't actually require those things.

  22. docs/reviewbot/tools/cargo.rst (Diff revision 2)
     
     

    We should link to rustup.rs

  23. 
      
ilaw
david
Review request changed

Status: Discarded

Loading...