django-and-python
code-review-doctorbotwould suggest these changes right inside your pull requests on GitHub.
config/settings.py
41
41
42
42
# https://docs.djangoproject.com/en/dev/ref/settings/#append-slash
43
43
APPEND_SLASH = True

Stating defaults add complexity when reading the code but does not change Django's behaviour.

Read more
Suggested changes
-
APPEND_SLASH = True
44
44
45
45
# Application definition
62
62
    'whitenoise.middleware.WhiteNoiseMiddleware',

Your website is vulnerable to a number of common hacker attacks because MIDDLEWARE setting is missing django.middleware.security.SecurityMiddleware.

Read more

The order of middleware affections the outcome. Some middleware are dependant on the functionality of other middleware. For example a middleware that requires usage of request.session should come after the SessionMiddleware.

Read more
Suggested changes
+
    'django.middleware.security.SecurityMiddleware',
Expand 12 lines ...
63
63
    'django.contrib.sessions.middleware.SessionMiddleware',
64
64
    'django.middleware.cache.UpdateCacheMiddleware',
65
65
    'django.middleware.common.CommonMiddleware',
66
66
    'django.middleware.cache.FetchFromCacheMiddleware',
67
67
    'django.middleware.csrf.CsrfViewMiddleware',
68
68
    'django.contrib.auth.middleware.AuthenticationMiddleware',
69
69
    'django.contrib.messages.middleware.MessageMiddleware',
70
70
    'django.middleware.clickjacking.XFrameOptionsMiddleware',
71
71
]
72
72
73
73
ROOT_URLCONF = 'config.urls'
74
74
75
75
TEMPLATES = [

DIRS must be absolute paths. Relative paths will not work.

Read more
76
76
    {
77
77
        'BACKEND': 'django.template.backends.django.DjangoTemplates',
78
78
        'DIRS': ['core/templates'],
79
79
        'APP_DIRS': True,
80
80
        'OPTIONS': {
81
81
            'context_processors': [
82
82
                'django.template.context_processors.debug',
83
83
                'django.template.context_processors.request',
84
84
                'django.template.context_processors.i18n',
85
85
                'django.contrib.auth.context_processors.auth',
86
86
                'django.contrib.messages.context_processors.messages',
87
87
                'staticpages.context_processors.get_pages',
88
88
                'staticpages.context_processors.get_categories',
89
89
                'staticpages.context_processors.get_urls',
90
90
            ],
91
91
        },
92
92
    },
93
93
]
94
94
95
95
# Static files served with Whitenoise and AWS Cloudfront
119
119
SECURE_CONTENT_TYPE_NOSNIFF = True
120
120
121
121
USE_I18N = True

Stating defaults add complexity when reading the code but does not change Django's behaviour.

Read more
Suggested changes
-
USE_I18N = True
122
122
123
123
DIRECTORY_CONSTANTS_URL_GREAT_DOMESTIC = env.str(
148
148
SSO_PROFILE_URL = 'https://profile.com'

Your website is vulnerable because the CSRF_COOKIE_SECURE setting is not set - so hackers have an easier time stealing your CSRF cookies on HTTP connections, allowing them to circumvent your CSRF protection.

Read more

Your website is vulnerable because the SESSION_COOKIE_SECURE setting is not set - so hackers have an easier time stealing your users' session cookies on HTTP connections.

Read more
Suggested changes
+
+
+
SESSION_COOKIE_SECURE = os.getenv('SESSION_COOKIE_SECURE_ENABLED') != 'False'
+
+
CSRF_COOKIE_SECURE = os.getenv('CSRF_COOKIE_SECURE_ENABLED') != 'False'
apps/aarvark/tests.py
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 ...
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()
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/students.py
5
5
6
6
class StudentPerformance(models.Model):
7
7
    stident_id = models.CharField(max_length=100, primary_key=True, unique=False)

Primary key must be unique by definition: it's a field that uniquely identifies the record.

Read more

Stating defaults add complexity when reading the code but does not change Django's behaviour.

Read more
Suggested changes
-
    stident_id = models.CharField(max_length=100, primary_key=True, unique=False)
+
    stident_id = models.CharField(max_length=100, primary_key=True)
Expand 2 lines ...
8
8
    registration_id = models.CharField(max_length=10, db_index=True)
9
9
    academic_year = models.IntegerField()
14
14
    courses_registration_validated = models.NullBooleanField(null=True)

NullBooleanField is deprecated and will be removed a future version of Django.

Read more
Suggested changes
-
    courses_registration_validated = models.NullBooleanField(null=True)
+
    courses_registration_validated = models.BooleanField(null=True)
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)
Use our GitHub pull request bot to prevent issue like these getting into your codebase.

Filter

Issues