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

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

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.

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

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)

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

This should probably be dedented one space.

This should use single quotes.

Let's use double quotes around "build"

Let's add a trailing comma to this line.

This should use single quotes.

Let's add a trailing comma to this line.

This should use single quotes.

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

Let's add a trailing comma to this line.

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

Let's add a trailing comma to this line.

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

Let's add a trailing comma to this line.

Please add a blank line between these two.

Add a blank line between these two please.

The comma within the function call here can go away.

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

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

Can you update the screenshot to show the latest text?

Is this a long enough timeout?

I just realized, we should use shlex.split(build_type) here, to parse it as a shell would. Otherwise if there are quoted arguments it might not be correct.

Summary should say "Review Bot", rather than "ReviewBot." We've altered the name a bit, but still have places where we have the old version.

Something seems to have gone wrong with the hange description. Guessing it's because there's no blank line before or after the list, which is needed for Markdown so it doesn't turn into italics.

"fbinfer" or "FBInfer"?

How about putting the string inside of the () on the next line, indented 4 spaces? That'll give it breathing room. Like:

{
    ...
    'help_text': (
        'Include a ...'
        '...'
    ),
}

This can be one statement.

You'll want a try/except around this, since it can fail.

Typo: "compilation".
The comment could use an adjustment. I wasn't sure at first glance what it was saying.

How about:

build_type = 'make'
settings['build_type'] = build_type


This is a bit cleaner, since you're not having to store a value and then look it up immediately after.

It's best to use += instead of .extend(). Operators are faster to invoke than functions.

Better to use += here too.

And here.

This will need a try/except.

This becomes a bit easier to see as one concrete operation if you do:

execute(['infer', 'run', '--'] +
        shlex.split(build_type) +
        [path])

+= here as well.

This needs a try/except.

fp is the standard variable for a file pointer when opening a file.
You should also specify an explicit mode parameter.
One last thing: This can fail (due to non-existence or permissions issues), so you will need to capture exceptions here. Maybe move this into the try.

Can we break after this match?

Formatting will be more consistent with our standards by doing:

f.comment('Bug Type: %s\n%s: %s'
          % (entry['bug_type_hum'],
             entry['severity']
             entry['qualifier']),
          entry['line'])


Or, maybe better:

f.comment('Bug Type: %(bug_type_hum)s\n'
          '%(severity)s %(qualifier)s'
          % entry,
          entry['line'])


By the way, is it bug_type_hum or bug_type_num?

No blank line here.

I just noticed a pretty significant bug with the multi_file_build_types. It will require some reworking, but other than that much of this implementation looks very solid.

If you make the change to handle_files that I suggested below, then cmake should be added to this list as well.

By my reading of the code, if the build type is one of multi_file_build_types, we'll never load the report and create comments.
I think perhaps we should split things up so that this method is very simple:

if settings['build_type'] in self.multi_file_build_types:
    self.run_multi_file_build(files)
else:
    for f in files:
        self.run_single_file_build(f)


We can then have run_multi_file_build do all the infer_cmd work that's done above here (plus actually load report.json and create comments for any files that are changed), and run_single_file_build would look a lot like what your current handle_file implementation looks like.

Would it be better to initialize build_type in __init__ instead of defining it in the method?

Same comment as in the other CR.

^

^

Ship It!