Trusted by over 3000 teams to securely fix bugs and improve Python code. Check your codebase for free instantly.
View demoWe've improved thousands of codebases including
Chrome
PyTorch
Sentry
Tensorflow
Firefox
It caught malformed test assertions that were incorrect. The automated codebase audit tool is so handy!
Jared Lockhart, Senior Software Engineer at MozillaExtremely positive. Suggested useful changes, giving our senior developers time back.
Jon Atkinson, Technical Director at GiantUncovered some bugs in the tests - thanks a lot!
Patrick von Platen, Research Engineer at Hugging FaceThis is a sample of the kind of advice you can expect from Code Review Doctor.
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.
- | 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
.
- | 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
.
- | 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
.
- | 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
.
- | 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.
- | 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- | 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- | 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.
- | 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
.
- | 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
.
- | 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.
- | 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- | 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 ... |
14 | 14 | "h3" |
Missing commas in tuples results in implicit string concatenation. Probably not what you intended to do.
Read more- | "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()
.
- | 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- | 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- | 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
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()
.
- | 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.
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.
64 | 64 | _data = json.loads(src.read()) |
json.load(f)
simplifies reading JSON from disk.
- | _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.
- | with open(self._conf_path, "w") as conf_dest: |
+ | with open(self._conf_path, "w", encoding="utf_8") as conf_dest: |
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.
- | 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.
- | 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.
- | except ValueError or SyntaxError: | |||
+ | except (ValueError, SyntaxError): | |||
Expand 2 lines ... |
8 | 8 | ⠀ | |
9 | 9 | ||
10 | 10 | @dataclass |
Use frozen=True
to make the dataclasses
immutable and hashable.
- | @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
- | 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 !=
.
- | 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()
.
- | 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()
.
- | 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()
.
- | 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 more46 | 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 more55 | 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
.
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.
- | with open('rack_elevation.css', 'w') as css_file: |
+ | with open('rack_elevation.css') as css_file: |
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.
- | 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- | 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.
- | 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.
- | f.write(json.dumps(result)) |
+ | json.dump(result, f) |
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.
- | with open("sample-chinese.txt", encoding="utf-8") as input_file: | |||
+ | with open("sample-chinese.txt", encoding="big5") as input_file: | |||
Expand 2 lines ... |