FBInfer for ReviewBot

Review Request #11202 — Created Sept. 25, 2020 and updated

jblazusi
ReviewBot
master
0bda1d6...
reviewbot, students
ceciliawei, jace

FBInfer Tool is now working for all the following build configurations:
Android
Apache Ant
Buck
C
CMake
Gradle with and without a Wrapper
iOS
Java
Make
Maven
Objective-C
XCode
The usability of this tool is largely dependent on the setup of a
user's ReviewBoard environment. Meaning that if a user would like to
use ReviewBot to test an Android project. The user must have a working
Android SDK in the server instance in which the ReviewBot is pulling
down the repository. Setups for each particular environment will be
offered in the documentation.

Each build configuration was tested manually with its own environment
and repository. More information about the tests can be found in
Week 3 and Week 4 of my Notion journal: https://bit.ly/351ttRQ.

Loading file attachments...

Description From Last Updated

F401 'reviewbot.utils.process.execute' imported but unused

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E124 closing bracket does not match visual indentation

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

F401 'reviewbot.utils.process.execute' imported but unused

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

Could you add a comment at the start of the file? e.g. Following the doc8.py, """Review Bot tool to run ...

ceciliaweiceciliawei

Yep, that is definitely an artifact from shuffling my code around. Good catch, seems that flake8 did not complain about ...

jblazusijblazusi

Were you intended to put the command in the end? Shouldn't it just be list.extend(...)?

ceciliaweiceciliawei

Since this change is in another CR for review, is it possible to remove the change from this CR? So ...

ceciliaweiceciliawei

I was reading something related to json then I thought about this code review. Would it be helpful to put ...

ceciliaweiceciliawei

This should probably be dedented one space.

daviddavid

This should use single quotes.

daviddavid

Let's use double quotes around "build"

daviddavid

Let's add a trailing comma to this line.

daviddavid

This should use single quotes.

daviddavid

Let's add a trailing comma to this line.

daviddavid

This should use single quotes.

daviddavid

Let's use double quotes around "iphonesimulator", which lets us avoid the \\ characters.

daviddavid

Let's add a trailing comma to this line.

daviddavid

Can you add periods to the end of the comments in here? This line and others below.

daviddavid

Let's add a trailing comma to this line.

daviddavid

There are a bunch of places where we use settings['build_type']. Perhaps pull that out into a build_type variable?

daviddavid

Let's add a trailing comma to this line.

daviddavid

Please add a blank line between these two.

daviddavid

Add a blank line between these two please.

daviddavid

The comma within the function call here can go away.

daviddavid

I don't think we need to list the file here, since the diff comment will be attached to the file ...

daviddavid

Just for consistency, can we call the entry point fbinfer?

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

flake8

jblazusi
Review request changed

Commit:

-12d61818c8fb763755274cc4d11c03981905235a
+8c6e253b460c7e78839d02260874e3096e34af91

Diff:

Revision 2 (+56)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

jblazusi
Review request changed

Commit:

-8c6e253b460c7e78839d02260874e3096e34af91
+bcd3d151388605cb49da1482c3d915c3de48c42f

Diff:

Revision 3 (+106 -1)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

jblazusi
jblazusi
jblazusi
  1. 
      
    1. Fixing the wrapping of the description to be between 70-75 characters.

  2. 
      
jblazusi
ceciliawei
  1. 
      
  2. bot/reviewbot/tools/fbinfer.py (Diff revision 4)
     
     

    Could you add a comment at the start of the file? e.g. Following the doc8.py,

    """Review Bot tool to run doc8."""
    
    from __future__ import unicode_literals
    
    import logging
    
    from reviewbot.tools import Tool
    from reviewbot.utils.process import execute, is_exe_in_path
    
    1. Actually, looks like this is not consistent among the tools.

    2. This is a great suggestion, and yes, we definitely want that!

      A lot of code in our codebases predate our modern doc standards. When appropriate, we try to update existing code (for instance, we'll add a docstring to a file being modified if it's missing one, or modernize a function's docs if we're adding a new argument). All new code needs to follow the modern doc conventions, which will over time help lead to consistency.

    3. I included this """Review Bot tool to run FBInfer.""" at the top of the FBInfer file. Not sure if anything else needs to be included.

  3. 
      
jblazusi
  1. 
      
  2. 
      
jblazusi
jblazusi
jblazusi
jblazusi
ceciliawei
  1. 
      
  2. bot/reviewbot/tools/fbinfer.py (Diff revision 5)
     
     

    Were you intended to put the command in the end? Shouldn't it just be list.extend(...)?

    1. I meant comma rather than command. Sorry about the confusion.

  3. extension/reviewbotext/resources.py (Diff revision 5)
     
     

    Since this change is in another CR for review, is it possible to remove the change from this CR? So that the description could reflect the content better.

    1. I was able to fix this issue with Christian's help.

  4. 
      
ceciliawei
  1. 
      
  2. 
      
jblazusi
  1. 
      
  2. bot/reviewbot/tools/fbinfer.py (Diff revision 5)
     
     

    Yep, that is definitely an artifact from shuffling my code around. Good catch, seems that flake8 did not complain about this.

  3. 
      
jblazusi
jblazusi
jblazusi
ceciliawei
  1. 
      
  2. bot/reviewbot/tools/fbinfer.py (Diff revision 7)
     
     

    I was reading something related to json then I thought about this code review. Would it be helpful to put the json.load(file) in a try/catch block and add loggings? e.g.
    try: # json.load except Exception as e: logger.exception('Could not read the json: %s', e)

    1. I think that this could potentially help with tracing errors, especially if FBInfer made significant changes to their commands or perhaps generates bad json output.

  3. 
      
jblazusi
  1. 
      
    1. Included a try and except block for loading the json as an extra pre-caution

  2. 
      
jblazusi
Review request changed

Commit:

-c7a7924ac757939c14fff1bab0d6050010011046
+0bda1d630a5111f48210af2b61aae8c61e9694e0

Diff:

Revision 8 (+182)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
david
  1. This is looking great. There are a bunch of comments here but they're pretty much all just style nitpicks.

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

    This should probably be dedented one space.

  3. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     

    This should use single quotes.

  4. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     

    Let's use double quotes around "build"

  5. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     

    Let's add a trailing comma to this line.

  6. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     

    This should use single quotes.

  7. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     

    Let's add a trailing comma to this line.

  8. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     

    This should use single quotes.

  9. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
     

    Let's use double quotes around "iphonesimulator", which lets us avoid the \\ characters.

  10. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     

    Let's add a trailing comma to this line.

  11. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     

    Can you add periods to the end of the comments in here? This line and others below.

  12. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     

    Let's add a trailing comma to this line.

  13. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     

    There are a bunch of places where we use settings['build_type']. Perhaps pull that out into a build_type variable?

  14. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     

    Let's add a trailing comma to this line.

  15. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
     

    Please add a blank line between these two.

  16. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
     

    Add a blank line between these two please.

  17. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     

    The comma within the function call here can go away.

  18. bot/reviewbot/tools/fbinfer.py (Diff revision 8)
     
     
     
     
     
     
     
     
     

    I don't think we need to list the file here, since the diff comment will be attached to the file already.

  19. bot/setup.py (Diff revision 8)
     
     

    Just for consistency, can we call the entry point fbinfer?

  20. 
      
Loading...