[WIP] Adding nyc tool to ReviewBot

Review Request #10195 — Created Oct. 4, 2018 and updated

alextechcc
ReviewBot
master
10095
bf2de3b...
reviewbot, students
[WIP] Adding nyc tool to ReviewBot


  • 0
  • 0
  • 11
  • 3
  • 14
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

alextechcc
alextechcc
brennie
  1. 
      
  2. bot/reviewbot/tools/nyc.py (Diff revision 3)
     
     
     

    Nit: blank line between these.

  3. bot/reviewbot/tools/nyc.py (Diff revision 3)
     
     
     

    This doesn't need to be in the with block.

  4. bot/reviewbot/tools/nyc.py (Diff revision 3)
     
     

    Does nyc work on JSX (.jsx)? Does it work on TypeScript .ts/.tsx?

  5. bot/reviewbot/tools/nyc.py (Diff revision 3)
     
     
  6. bot/reviewbot/tools/nyc.py (Diff revision 3)
     
     
     

    You will want to use the os.path module here. / is not guaranteed to be the path separator on every platform. Technically you could run the reviewbot worker on Windows if you needed to run a Windows-only tool.

    import os
    
    # ...
    
    file_coverage = coverage[os.path.join(os.getcwd(), path)]
    
  7. bot/reviewbot/tools/nyc.py (Diff revision 3)
     
     

    Can we pass in cwd from the parent method? If there are lots of files, that is a lot of calls to os.getcwd(), which isn't going to change.

  8. 
      
alextechcc
brennie
  1. 
      
  2. bot/reviewbot/tools/nyc.py (Diff revision 4)
     
     

    You do not need to append the path sep. That is what os.path.join is for.

  3. bot/reviewbot/tools/nyc.py (Diff revision 4)
     
     

    os.path.join(t) is just t. You don't need it.

  4. bot/reviewbot/tools/nyc.py (Diff revision 4)
     
     

    In what case does f.get_patched_file_path() return None ?

    1. I saw it included in most other tools, looks like it returns None on an API error.

  5. bot/reviewbot/tools/nyc.py (Diff revision 4)
     
     

    Comments should be full sentences, i.e., they should begin with a capital letter and end with a period.

  6. bot/reviewbot/tools/nyc.py (Diff revision 4)
     
     
     

    If you're using the first_line logic you added in your other patch, you'll want to mark this review request as depending on the other.

  7. 
      
alextechcc
alextechcc
Review request changed

Change Summary:

Rebased on 10095 (Add ability for bots to make general comments)

Depends On:

-10219 - [WIP] Potential whole-file fix for file modification when commenting
+10095 - Adding the ability for bots to make general comments

Commit:

-121a4f862aadd3d5ee257959ffe53686bab9b45d
+bf2de3b357ab2bd534159c6d94dbb5bf1e72fa58

Diff:

Revision 5 (+165)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...