Fix more bugs during code review

Trusted by over 3000 teams to securely fix bugs and improve Python code. Check your codebase for free instantly.

View demo

We've improved thousands of codebases including

  • Chrome

    Chrome

  • PyTorch

    PyTorch

  • Sentry

    Sentry

  • Tensorflow

    Tensorflow

  • Firefox

    Firefox

It caught malformed test assertions that were incorrect. The automated codebase audit tool is so handy!

Jared Lockhart, Senior Software Engineer at Mozilla

Extremely positive. Suggested useful changes, giving our senior developers time back.

Jon Atkinson, Technical Director at Giant

Uncovered some bugs in the tests - thanks a lot!

Patrick von Platen, Research Engineer at Hugging Face

Works with tools you use

Read about how it works.

A quick example

This is a sample of the kind of advice you can expect from Code Review Doctor.

code-review-doctorbotwould suggest these changes right inside your pull requests on GitHub.
apps/aarvark/tests.py
11
11
def test_settings():
12
12
    assert settings.VALUE > 10, '{settings.VALUE} is too high'

An f-string will not work if the f prefix is missing.

Read more
Suggested changes
-
    assert settings.VALUE > 10, '{settings.VALUE} is too high'
+
    assert settings.VALUE > 10, f'{settings.VALUE} is too high'
13
13
25
25
        allocate(membership)
26
26
        assert membership.condition is not None
27
27
        assert membership.observation_set.all().count() is 0

Primitive data types such as strings and integers should be compared using == and != rather than is and is not.

Read more
Suggested changes
-
        assert membership.observation_set.all().count() is 0
+
        assert membership.observation_set.all().count() == 0
Expand 2 lines ...
28
28
29
29
        membership.add_observations()
30
30
        assert membership.observation_set.all().count() is 1

Primitive data types such as strings and integers should be compared using == and != rather than is and is not.

Read more
Suggested changes
-
        assert membership.observation_set.all().count() is 1
+
        assert membership.observation_set.all().count() == 1
Expand 2 lines ...
31
31
32
32
    def test_title_content(self):
34
34
        expected_object_name = f'{topic.title}'
35
35
        count = Topic.objects.count()
36
36
        self.assertEquals(expected_object_name, 'Test topic')

Don't use deprecated TestCase assertions like self.assertEquals. Use their modern counterpart instead like self.assertEqual.

Read more
Suggested changes
-
        self.assertEquals(expected_object_name, 'Test topic')
+
        self.assertEqual(expected_object_name, 'Test topic')
37
37
        self.assertEquals(count,1)

Don't use deprecated TestCase assertions like self.assertEquals. Use their modern counterpart instead like self.assertEqual.

Read more
Suggested changes
-
        self.assertEquals(count,1)
+
        self.assertEqual(count,1)
Expand 2 lines ...
38
38
39
39
    def test_one_object(self):
40
40
        self.assertTrue(len(DataCollector().profiles), 1)

assertTrue is not for comparing arguments. Consider assertEqual or others instead.

Read more
Suggested changes
-
        self.assertTrue(len(DataCollector().profiles), 1)
+
        self.assertEqual(len(DataCollector().profiles), 1)
Expand 2 lines ...
41
41
42
42
    def test_pivot_dtaccessor(self):
67
67
            }
68
68
        )
69
69
        df["dt1"] = df["dt1"].apply(lambda d: pd.Timestamp(d))

Avoid unnecessarily wrapping a function in a lambda

Read more
Suggested changes
-
        df["dt1"] = df["dt1"].apply(lambda d: pd.Timestamp(d))
+
        df["dt1"] = df["dt1"].apply(pd.Timestamp)
70
70
        df["dt2"] = df["dt2"].apply(lambda d: pd.Timestamp(d))

Avoid unnecessarily wrapping a function in a lambda

Read more
Suggested changes
-
        df["dt2"] = df["dt2"].apply(lambda d: pd.Timestamp(d))
+
        df["dt2"] = df["dt2"].apply(pd.Timestamp)
Expand 2 lines ...
71
71
72
72
        result = pivot_table(
83
83
        )
84
84
        self.assertTrue(serializer.data[0]["is_control"])
85
85
        self.assertFalse(any([b["is_control"] for b in serializer.data[1:]]))

all and any can take a generator, so constructing a list first may be unnecessary.

Read more
Suggested changes
-
        self.assertFalse(any([b["is_control"] for b in serializer.data[1:]]))
+
        self.assertFalse(any(b["is_control"] for b in serializer.data[1:]))
86
86
        self.assertEqual(sorted_treatment_ids, [b["id"] for b in serializer.data[1:]])
87
87
95
95
        ).render(self.mocked_context)
96
96
97
97
        self.assertTrue('{"name": "Tom Waits"}' in out)

assertIn and assertNotIn provide more helpful failure messages than assertTrue or assertFalse.

Read more
Suggested changes
-
        self.assertTrue('{"name": "Tom Waits"}' in out)
+
        self.assertIn('{"name": "Tom Waits"}', out)
Expand 2 lines ...
98
98
99
99
    def test_can_be_hashed_including_non_ascii(self):
106
106
            user=self.user)
107
107
        hash = a.generate_hash()
108
108
        self.assertTrue(hash is not None)

assertIsNone and assertIsNotNone provide more helpful failure messages than assertTrue or assertFalse.

Read more
Suggested changes
-
        self.assertTrue(hash is not None)
+
        self.assertIsNotNone(hash)
Expand 2 lines ...
109
109
110
110
    def test_is_valid_call_prerequisite_validators(self, mock_prerequisite_validator):
111
111
        prerequisite_string = "LOSIS1452 OU LPORT5896"
112
112
        program_tree = ProgramTreeFactory(),

A comma after a value turns it into tuple. Probably not what you intended to do.

Read more
Suggested changes
-
        program_tree = ProgramTreeFactory(),
+
        program_tree = ProgramTreeFactory()
113
113
114
114
    def test_is_valid_call_prerequisite_validators(self, mock_prerequisite_validator):

Duplicate names for tests results in some tests being skipped.

Read more
Suggested changes
-
    def test_is_valid_call_prerequisite_validators(self, mock_prerequisite_validator):
+
    def test_is_valid_call_prerequisite_validators_two(self, mock_prerequisite_validator):
Expand 2 lines ...
apps/aarvark/utils.py
14
14
    "h3"

Missing commas in tuples results in implicit string concatenation. Probably not what you intended to do.

Read more
Suggested changes
-
    "h3"
+
    "h3",
Expand 2 lines ...
15
15
    "h1",
16
16
    "h2",
26
26
27
27
28
28
PRODUCTS = list()

Using list/set/dict literal syntax is simpler and computationally quicker than calling list(), set(), or dict().

Read more
Suggested changes
-
PRODUCTS = list()
+
PRODUCTS = []
29
29
30
30
32
32
33
33
    def __init__(self, attrs=None, choices=(), disabled_choices=()):
34
34
        super(ProviderLinkWidget, self).__init__(attrs, choices=choices)

It's unnecessary to use arguments when calling super for the parent class.

Read more
Suggested changes
-
        super(ProviderLinkWidget, self).__init__(attrs, choices=choices)
+
        super().__init__(attrs, choices=choices)
35
35
        self.disabled_choices = disabled_choices
36
36
37
37
    def get_required_permission(self):
38
38
        return f'virtualization.add_vminterface'

f-string is unnecessary here as there are no placeholders in the string.

Read more
Suggested changes
-
        return f'virtualization.add_vminterface'
+
        return 'virtualization.add_vminterface'
Expand 2 lines ...
39
39
40
40
    def get_attach_path(self) -> Union[Path, None]:

This could be simplified by using typing.Optional

Read more
43
43
44
44
def get_size(vals):
45
45
    return len(list(vv.strip() for vv in vals.split(splitter) if vv.strip()))

Using list and dict comprehension is simpler and computationally quicker than calling list() and dict().

Read more
Suggested changes
-
    return len(list(vv.strip() for vv in vals.split(splitter) if vv.strip()))
+
    return len([vv.strip() for vv in vals.split(splitter) if vv.strip()])
46
46
47
47
49
49
    with open(file_h, 'rb') as file_h:
50
50
        buf = file_h.readline()
51
51
    return file_h.readline()

Accidentally trying to read or write files that are closed will cause a ValueError at runtime.

Read more
61
61
            if os.path.exists(node_settings):
62
62
                _data = {}
63
63
                with open(node_settings) as src:

Not specifying encoding when reading a file can cause UnicodeDecodeError because Python assumes the file is encoded with the OS's default text encoding, but that's often an invalid assumption.

Read more
64
64
                    _data = json.loads(src.read())

json.load(f) simplifies reading JSON from disk.

Read more
Suggested changes
-
                    _data = json.loads(src.read())
+
                    _data = json.load(src)
68
68
        with open(self._conf_path, "w") as conf_dest:

Not specifying encoding when writing a file can cause UnicodeEncodeError because Python assumes the string's characters can fit in the OS's default text encoding, but that's often an invalid assumption.

Read more
Suggested changes
-
        with open(self._conf_path, "w") as conf_dest:
+
        with open(self._conf_path, "w", encoding="utf_8") as conf_dest:
apps/aarvark/mixins.py
3
3
# ProbablyMeantTuple
4
4
5
5
class LoginRequiredMixin(object):

A class is inherited from object by default, so explicitly inheriting from object is redundant. Removing it keeps the code simpler.

Read more
Suggested changes
-
class LoginRequiredMixin(object):
+
class LoginRequiredMixin:
Expand 2 lines ...
6
6
7
7
    redirect_field_name = 'next'
8
8
    login_url = None
9
9
    fields = ('title')

A tuple with only one element must end with a comma. Python won't know it's a tuple without the comma. Probably not what you intended to do.

Read more
Suggested changes
-
    fields = ('title')
+
    fields = ('title',)
Expand 2 lines ...
10
10
11
11
    def format_product_option(self, product_string):
13
13
        try:
14
14
            list(map(int, products))
15
15
        except ValueError or SyntaxError:

Catch multiple exception types using a tuple, not with or operator.

Read more
Suggested changes
-
        except ValueError or SyntaxError:
+
        except (ValueError, SyntaxError):
Expand 2 lines ...
apps/aarvark/cache.py
8
8
9
9
10
10
@dataclass

Use frozen=True to make the dataclasses immutable and hashable.

Read more
Suggested changes
-
@dataclass
+
@dataclass(frozen=True)
Expand 2 lines ...
11
11
class Cache:
12
12
    name: str
14
14
15
15
16
16
class ArgInfo(NamedTuple):

Consider using the dataclass here instead for simplicity, performance gains, and consistency

Read more
Suggested changes
-
class ArgInfo(NamedTuple):
+
@dataclass(frozen=True)
+
class ArgInfo:
    args: List[Value]
17
17
    args: List[Value]
18
18
    arg_names: List[Optional[str]]
23
23
    global _caches
24
24
    caches = _caches
25
25
    if caches == None:

Singleton data types such as booleans should be compared using is and is not rather than == and !=.

Read more
Suggested changes
-
    if caches == None:
+
    if caches is None:
Expand 2 lines ...
26
26
        pidCache = dict((u.pid, u) for u in _databaseQuery().all())

Using list and dict comprehension is simpler and computationally quicker than calling list() and dict().

Read more
Suggested changes
-
        pidCache = dict((u.pid, u) for u in _databaseQuery().all())
+
        pidCache = {u.pid: u for u in _databaseQuery().all()}
Expand 2 lines ...
27
27
        usernameCache = dict((u.username, u) for u in pidCache.values())

Using list and dict comprehension is simpler and computationally quicker than calling list() and dict().

Read more
Suggested changes
-
        usernameCache = dict((u.username, u) for u in pidCache.values())
+
        usernameCache = {u.username: u for u in pidCache.values()}
Expand 2 lines ...
28
28
        idCache = dict((u.id, u) for u in pidCache.values())

Using list and dict comprehension is simpler and computationally quicker than calling list() and dict().

Read more
Suggested changes
-
        idCache = dict((u.id, u) for u in pidCache.values())
+
        idCache = {u.id: u for u in pidCache.values()}
Expand 2 lines ...
29
29
        caches = (pidCache, usernameCache, idCache)
30
30
        _caches = caches
36
36
    def open(self, parent=None, detail_item=None, external=False):
37
37
        url = 'https://books.google.com/books'
38
38
        if True or external or self.config.get('open_external', False):

This code will always evaluate the same value regardless of the inputs.

Read more
46
46
    def __str__(self):
47
47
        course = self.course
48
48
        return "{} - {} - {}".format(self._type, course.id, course.subject.name)

f-string is easier to read, write, and less computationally expensive than legacy string formatting.

Read more
55
55
        with open('rack_elevation.css', 'w') as css_file:

Read and write calls will fail at runtime if open is called with incorrect modes.

Read more

Not specifying encoding when reading a file can cause UnicodeDecodeError because Python assumes the file is encoded with the OS's default text encoding, but that's often an invalid assumption.

Read more
Suggested changes
-
        with open('rack_elevation.css', 'w') as css_file:
+
        with open('rack_elevation.css') as css_file:
apps/aarvark/ref_proxy.py
12
12
    _invoke_func = _local_invoke
13
13
    # Bypass ScriptModules when checking for async function attribute.
14
14
    bypass_type = issubclass(rref_type, torch.jit.ScriptModule) or issubclass(

issubclass can take multiple types, so there is no need to call them multiple times for each type.

Read more
Suggested changes
-
    bypass_type = issubclass(rref_type, torch.jit.ScriptModule) or issubclass(
+
    bypass_type = issubclass(rref_type, (torch.jit.ScriptModule, torch._C.ScriptModule))
-
        rref_type, torch._C.ScriptModule
-
    )
    import ipdb; ipdb.set_trace()
15
15
        rref_type, torch._C.ScriptModule
16
16
    )
17
17
    import ipdb; ipdb.set_trace()

Don't commit breakpoints. This will pause execution in prod and break the app.

Read more
Suggested changes
-
    import ipdb; ipdb.set_trace()
18
18
19
19
23
23
    conf = {}
24
24
    with open(self._conf_path, "r", encoding='utf-8') as conf_src:
25
25
        conf = json.loads(conf_src.read())

json.load(f) simplifies reading JSON from disk.

Read more
Suggested changes
-
        conf = json.loads(conf_src.read())
+
        conf = json.load(conf_src)
26
26
    return conf
27
27
31
31
        f.write(json.dumps(result))

json.dump(f) simplifies writing JSON to disk.

Read more
Suggested changes
-
        f.write(json.dumps(result))
+
        json.dump(result, f)
apps/aarvark/api2.py
10
10
        context = super().get_context_data(**kwargs)
11
11
12
12
        with open("sample-chinese.txt", encoding="utf-8") as input_file:

Specifying incorrect encoding when reading a file can cause UnicodeDecodeError if the contents of the file is incompatible with the specified encoding.

Read more
Suggested changes
-
        with open("sample-chinese.txt", encoding="utf-8") as input_file:
+
        with open("sample-chinese.txt", encoding="big5") as input_file:
Expand 2 lines ...
Use our GitHub pull request bot to prevent issue like these getting into your codebase.

Check your repository instantly for free