Skip to content

Latest commit

 

History

History
1437 lines (1021 loc) · 62.8 KB

CONTRIBUTING.md

File metadata and controls

1437 lines (1021 loc) · 62.8 KB
Raw

Contribution guidelines

Table of contents

Code style guides

Table of contents

Generated with markdown-toc (and manually edited).

Global

Tip: The project's .editorconfig file may be used to configure a text editor to format code to (roughly) fit this style guide - especially when using an IntelliJ-based IDE, like PyCharm.

Max line length

150 (can be surpassed if it's impossible to wrap).

Empty/blank lines

All files (including committed third-party libraries, like jQuery) should end with exactly one empty line.

There should be no more than two empty lines in a row in a file. Exceptions to this are documentation, and markup languages (like HTML and Markdown).

MAKE vs. the word "make"

If writing the name of the organization (MAKE), write it in capital letters, to avoid confusion with the verb "make".

Git

Branch name

Use kebab-case.

The type of changes that the branch will contain, can be prefixed - with a / separating these parts of the branch name. Common name prefixes include:

  • feature/ - New feature
  • bug/ - Code changes linked to a known issue
  • cleanup/ - Cleaning up or refactoring code
  • fix/ - Other fixes to the codebase

Commit message

Use the verb conjugation (e.g. past, present or imperative) you prefer.

The first line of the commit message should concisely outline the changes in the commit. It should ideally be 50 characters or shorter.
Tip: if you find yourself using the word "and" in this summary, it's often a sign that the changes in the commit should be split into multiple commits.

Details that were omitted from the first line should be elaborated on and explained in subsequent lines (with an empty line between the first line, and the rest of the commit message). These lines should comply with the same length restriction as described in Max line length. Common things to include in this part of the commit message, are:

  • A more complete overview/description/explanation of the commit's changes - especially changes that are not immediately obvious
  • The reason for the changes, like if they've been discussed, or if the reason is not entirely straightforward
  • Links to things like where the code came from / was inspired by, documentation, further reading, etc.
  • Instructions for things that must/should be done when merging or checking out the commit
  • In general, things that make up the context of the changes - which could potentially be useful to have in the future for understanding why it was done a certain way, which other changes were necessary to facilitate the changes, which assumptions had to be made or which special conditions made the basis for the changes, etc.

Finally, here is an excellent blog post from GitHub on the usefulness of keeping a logical commit structure and descriptive commit messages, with reasoning, examples and tips: "Write Better Commits, Build Better Projects".

Python

For code in general

PEP8

In general, we follow the PEP 8 style guide.
Tip: PyCharm marks (most) PEP 8 violations with a yellow squiggly line. Additionally, PyCharm can easily fix most of these inconsistencies - and enforce various other parts of this style guide - using the Reformat Code feature (Ctrl+Alt+L/⌥⌘L by default) - possibly requiring some tweaking of the settings.

String quotation marks

Use ' for "code values" that are meant to be compared exactly against some other values. In short: if these strings are changed, things might break. Examples include:

  • Existing paths, like file paths for templates, or dotted module paths.
  • Names of variables or attributes, like object attributes, keyword arguments, or template context variables.

Use " for messages, names, etc. that are not supposed to be compared to existing values. In short: if these strings are changed, nothing should break. Examples include:

  • Text in a natural language, like messages that are shown to users or developers.
  • Path definitions - e.g. the first argument passed to the path() function - as these are not supposed to be directly referenced elsewhere (they should instead be inserted using the reverse() function or the url template tag) and can in principle be changed freely without altering the functionality in any way.
  • Docstrings.

If unsure, or if the case in question is in a grey area, use '.

This specific coding style is loosely based on this library's coding standards.

Quotation marks inside strings

Prefer using the proper Unicode characters for quotation marks in the language of the string in question (e.g. “ and ” in English, and « and » in Norwegian).

Avoid using the " (or \") character inside translated strings. Instead, use the Unicode characters mentioned above, or HTML character entities like ".

String concatenation

Prefer using f-strings for string concatenation; if an f-string is hard to read, extract inserted code to variables, and insert the variables instead.

For translation strings (using gettext or gettext_lazy), use the standard format() method for concatenation. For example:
_("{chant} Batman!").format(chant="NaN" * 15) (where gettext_lazy is imported as _).

Trailing commas

Always leave a trailing comma after the last element in a wrapped list/tuple/set/dictionary initialization expression or wrapped function/constructor call.

Operator wrapping

When wrapping an expression containing operators (like +, & and and), place the operators first on each wrapped line (instead of last on the previous lines). This also applies to similar things, like list comprehension expressions and conditional expressions (aka ternary operator).

To illustrate, a good example of wrapping the following code:

def func(long_condition_expr, other_long_expr, list_of_lists):
    if long_condition_expr and long_condition_expr:
        return other_long_expr + other_long_expr
    return [element for inner_list in list_of_lists for element in inner_list if element is not None]

can be:

def func(long_condition_expr, other_long_expr, list_of_lists):
    if (long_condition_expr
            and long_condition_expr):
        return (
                other_long_expr
                + other_long_expr
        )
    return [
        element
        for inner_list in list_of_lists
        for element in inner_list
        if element is not None
    ]

Imports

Group imports in three "paragraphs" (separated by an empty line) in the following order:

  1. Modules from Python's standard library
  2. Third-party modules
  3. Modules part of this project

Within each import group/paragraph, sort plain imports first, followed by from imports. Additionally, sort imports alphabetically, including names listed in from imports.
Tip: All of this can easily be done using PyCharm's Optimize Imports feature (Ctrl+Alt+O/⌃⌥O by default) - possibly requiring some tweaking of the settings.

All imports in a file that are from the same app as the mentioned file, should be relative (e.g. from .models import User or from .. import views).

For each module (file)

Empty/blank lines

Leave two empty lines between class and function (i.e. not method) definitions, and after all the imports in a module.

Folder/directory location

  • Tests should always be placed within a tests directory per app.
    • In addition to helping keep the file structure clean, this also makes writing various configuration files easier, like the ignore patterns in codecov.yml, and the exclude_patterns and tests_patterns in .codeclimate.yml.
  • Templates should be placed within an <app name> directory, within a templates directory per app.
    • For example:
      • app_name/templates/app_name/template_name.html
  • Static files (like CSS, JavaScript or image files) should be placed within a directory named after the file type/category, within an <app name> directory, within a static directory per app.
    • For example:
      • app_name/static/app_name/css/stylesheet.css
      • app_name/static/app_name/js/script.js
      • app_name/static/app_name/img/image.png
  • Template tags and filters should be placed within a templatetags directory per app.

Filename

Use snake_case.

Additionally:

  • modelfields.py should be the name of files containing custom model fields.
  • formfields.py should be the name of files containing custom form fields.
  • forms.py should be the name of files containing forms.
  • widgets.py should be the name of files containing form widgets.
  • widgets.py should be the name of files containing form widgets.
  • converters.py should be the name of files containing path converters.
  • signals.py should be the name of files containing model signals.
  • context_processors.py should be the name of files containing template context processors.
  • Test modules should be named with a test_ prefix.
Migration filename:

Migrations generated by Django that are named <index>_auto_<timestamp>, should be renamed to describe what the migration does.
Tip: If it's difficult to summarize what the migration does with a few words, it might be a sign that the migration should be split into multiple migrations.

A common naming pattern is <index>_<model name>_<field name>_<change description> (where <model name> is the lowercased model name without _ between words), but making the name understandable always takes priority. Example names include:

  • 0002_printer3dcourse_card_number
    • (The field card_number is added to the model Printer3DCourse.)
  • 0009_alter_user_last_name_max_length
    • (The max_length attribute of the last_name field of the User model is altered.)
  • 0012_rename_email_to_contact_email
    • (The email field of an unspecified model is renamed to contact_email; generally, basic details like the model name should be mentioned, but if it would be confusing to read, and the mentioned details can be implicitly understood, they can be omitted.)
  • 0014_member_user_set_null_on_delete
    • (The on_delete attribute of the user field on the Member model is changed to SET_NULL.)

For each class

Class name

Use PascalCase.

Field and method order

Sort the contents of a class in the following order:

  1. Constants
  2. Overridden fields
  3. New fields
  4. Managers (for model classes)
    • (E.g. objects = <Model name>QuerySet.as_manager().)
  5. Meta class (for model classes)
  6. Dunder (double underscore) methods
    • (E.g. __init__() or __str__().)
  7. Overridden methods
  8. New methods

For each view class

View class name

In general, names of views related to model objects should comply with one of the following patterns:

  • <Model name><Noun or verb>View - in most cases;
  • Admin<Model name><Noun or verb>View - for views that only admins should have access to;
  • API<Model name><Noun or verb>View - for views responding exclusively with JSON;
  • AdminAPI<Model name><Noun or verb>View - for views responding exclusively with JSON that only admins should have access to;
  • where:
    • <Model name> is the name of the model class that the view is related to.
    • <Noun or verb> is a word concisely outlining the contents of the view.
      • If the view inherits from one of Django's top-level generic views (ListView, DetailView, CreateView, UpdateView or DeleteView), the word should be the name of the generic view - without the View suffix.

(Note that the Admin/API/AdminAPI prefixes correspond with the path prefixes mentioned in Path prefixes.)

View class order

Sort views in the same order as they appear in the app's urls.py (see Path order).

View field order

A view inheriting from one of Django's generic views (the views that are part of the django.views.generic module) - and possibly also PermissionRequiredMixin - should have its fields sorted in the following order:

  1. permission_required
  2. model
  3. queryset
  4. form_class/fields
  5. template_name
  6. context_object_name
  7. extra_context
  8. success_url

For each function/method

Function/method name

Use snake_case.

Test methods should have a name that describes what they test and what the expected outcome is; for example test_event_delete_view_properly_deletes_related_objects() (for a DeleteView for events) or test_get_related_events_returns_expected_events() (for a model method named get_related_events()).

For each URL path

(Endpoint) path/route

In general, try to make paths as RESTful as possible.

Let all paths end with a / (except if the first argument to path() would have been just "/", in which case the argument should be an empty string).

Path naming conventions:

Use kebab-case, and concatenate compound nouns - as long as it's still fairly legible. Examples:

  • machinetypes/ is preferable over machine-types/;
  • softwareengineers/ is preferable over software-engineers/ - even if there's a double E, which makes it slightly less legible;
  • find-free-slots/ is preferable over findfreeslots/.

For paths that refer to views inheriting from one of the following generic views, the path specified is encouraged:

  • ListView: "<objects>/";
  • CreateView: "<objects>/add/";
  • DetailView: "<objects>/<pk>/";
  • UpdateView: "<objects>/<pk>/change/";
  • DeleteView: "<objects>/<pk>/delete/";
  • where:
    • <objects> (usually the model name) is lowercased and pluralized, without any word separator.
    • <pk> is a capture string for the primary key of the model - usually <int:pk>.

This makes the paths consistent with the ones used by the Django admin site, and the names of the corresponding default model permissions.

Path prefixes:

Prefix all endpoints' paths with the following patterns:

  • admin/ - if the endpoint/webpage should only be accessed by admins;
  • api/ - if the endpoint responds exclusively with JSON;
  • api/admin/ - if the endpoint responds exclusively with JSON and should only be accessed by admins.

See urlpatterns for admin/API view paths for how to manage this with urlpatterns.

The language path prefix is not affected by the above rules, so e.g. the English version of /api/admin/some-path/ being /en/api/admin/some-path/ is perfectly fine.

Other path grouping:

If a model is conceptually subordinated another model (e.g. an event occurrence model that is connected to an event model), the paths for the views related to that "sub-model" should be relative to the paths of the "super-model" - while still complying with the guidelines above. For example: event/<int:pk>/occurrences/<int:occurrence_pk>/change/.

Path name (the argument of path())

Use snake_case.

In general, paths should have the same name as the view they refer to (see View class name), but snake_cased and without the View suffix.

Path order

In general, sort paths based on the following order:

  1. ""
  2. "<objects>/"
  3. "<objects>/add/"
  4. "<objects>/<pk>/"
  5. "<objects>/<pk>/change/"
  6. "<objects>/<pk>/delete/"
  7. "<objects>/<pk>/<other paths>"
  8. "<other objects>/"

urlpatterns organization

In general, place paths with a common prefix inside separate lists, that are then include()d with the mentioned prefix.

To illustrate, a good example of reorganizing the following paths:

from django.urls import path


urlpatterns = [
    path("events/", ..., name='event_list'),
    path("events/add/", ..., name='event_create'),
    path("events/<int:pk>/", ..., name='event_detail'),
    path("events/<int:pk>/change/", ..., name='event_update'),
    path("events/<int:pk>/occurrences/", ..., name='event_occurrence_list'),
    path("events/<int:pk>/occurrences/<int:occurrence_pk>/", ..., name='event_occurrence_detail'),
]

would be:

from django.urls import include, path


event_occurrence_urlpatterns = [
    path("", ..., name='event_occurrence_list'),
    path("<int:pk>/", ..., name='event_occurrence_detail'),
]
specific_event_urlpatterns = [
    path("", ..., name='event_detail'),
    path("change/", ..., name='event_update'),
    path("occurrences/", include(event_occurrence_urlpatterns)),
]
event_urlpatterns = [
    path("", ..., name='event_list'),
    path("add/", ..., name='event_create'),
    path("<int:pk>/", include(specific_event_urlpatterns)),
]

urlpatterns = [
    path("events/", include(event_urlpatterns)),
]
urlpatterns for admin/API view paths:

For each app's urls.py file, place paths inside lists with the following names:

  • adminpatterns - if they refer to a view that only admins should have access to;
  • apipatterns - if they refer to a view responding exclusively with JSON;
  • adminapipatterns - if they refer to a view responding exclusively with JSON that only admins should have access to.

Each of these lists should now only contain paths referring to views with the corresponding class name prefixes listed in View class name.

These lists should then be imported in web/urls.py, and include()d in admin_urlpatterns, api_urlpatterns and admin_api_urlpatterns, respectively - with the same path route argument as the app's other paths. (This ensures that all paths start with the relevant admin/, api/ or api/admin/ prefix; see Path prefixes.) For example:

from django.urls import include, path

from news import urls as news_urls


admin_urlpatterns = [
    # Same path route as the other `news` app paths
    path("news/", include(news_urls.adminpatterns)),
]

admin_api_urlpatterns = [
    # Same path route as the other `news` app paths
    path("news/", include(news_urls.adminapipatterns)),
]

api_urlpatterns = [
    path("admin/", include(admin_api_urlpatterns)),
    # Same path route as the other `news` app paths
    path("news/", include(news_urls.apipatterns)),
]

urlpatterns = [
    path("admin/", include(admin_urlpatterns)),
    path("api/", include(api_urlpatterns)),
    # The `news` app's base path route argument ("news/")
    path("news/", include('news.urls')),
]

For each variable/field

Variable/field name

Use snake_case.

An exception to this is when the variable value is a reference to a specific model class - in which case, the variable should have the same name as the model it references; for example: InheritanceGroup = apps.get_model('groups', 'InheritanceGroup').

Model field definition arguments

Pass all arguments as keyword arguments.

Wrap all keyword arguments of relation fields (ForeignKey, OneToOneField and ManyToManyField) - i.e. place them on separate lines.

Model field keyword argument order:

Sort the keyword arguments in the following order:

  1. to
  2. on_delete
  3. null
  4. blank
  5. choices
  6. max_length
  7. default
  8. Other keyword arguments
  9. validators
  10. related_name
  11. verbose_name
  12. help_text
Model field argument value:
  • verbose_names should start with a lowercase letter, except if it's a name (like MAKE or GitHub).

Django templates / HTML / CSS

For code in general

String quotation marks

Use " for everything pure HTML and CSS.

Inside template tags and filters, use the same quotation marks as for Python. Examples include:

  • ' for:
    • url names
    • extends, include and static paths
  • " for:
    • translate/trans strings

Hex (color) code literals

Use uppercase A-F, to make them look more similar to the other digits.

For each file

Empty/blank lines

Leave two empty lines after all the extends and load template tags in a template.

Filename

Use snake_case.

Django template filenames:

In general, templates referred to by the template_name field of a view, should have the same name as that view (see View class name), but snake_cased and without the View suffix. An exception to this is if the view inherits from CreateView or UpdateView, in which case the <Noun or verb> in the view class name patterns should be form in the template name.

CSS filenames:

If a .css file is the "main" stylesheet for a specific template, it should have the same name as the template.

For each template block

Block order

Sort the blocks in a child template in the same order as they appear in the parent template(s).

Block name

Use snake_case.

endblock name

A block's name should always be repeated in the associated endblock template tag.

For each template variable

Template/context variable name

Use snake_case. This also applies to variables defined using the as keyword or the = syntax.

For each HTML tag

Empty HTML tag

Close empty tags with a />. For example: <br/> or <input type="submit"/>.

For each HTML attribute

Custom attribute name

Use kebab-case, and prefix the custom attribute with data-. (This enables getting or setting the attribute using jQuery's .data() function.)

class and id name

Use kebab-case.

Attribute order

If present, place id and class first - in that order.

For each CSS rule

Stylesheet rule order

Sort the rules in a stylesheet in (ideally) the same order as the elements they select appear in the template the stylesheet belongs to.

JavaScript

For code in general

String quotation marks

Use ". This does not apply to template strings, of course.

For each file

Filename

Use snake_case.

If a .js file is the "main" script for a specific template, it should have the same name as the template.

For each function

Function name

Use camelCase.

For each variable

Variable name

Use camelCase.

If the variable's value is a jQuery object, prefix the variable name with $.

Variable declaration (let/const/var)

Use:

  • const wherever possible,
  • var when defining or declaring variables in a template that will be used in a linked JavaScript file,
  • let in all other cases.

Code review guideline: Code smells

This list is meant as a guideline, and only provides hints that might enhance developers' ability to notice whether some code should be changed or not; there do exist scenarios where not getting rid of a code smell would be the most appropriate.

Table of contents

Generated with markdown-toc (and manually edited).

Global

For code in general

Outdated comments

Comments should be kept updated and consistent with the code they're commenting on.

Outdated tests

Tests should be kept up-to-date and relevant to the code they test.

Duplicated code

Code that's commonly (accidentally) duplicated, includes:

  • Code in each branch of an if statement
  • String literals
  • Code in templates
    • This can (almost always) be abstracted by extracting common code to a separate template, which is then included or extended.
  • Overridden view class methods
  • Initializing objects in test cases

Note that this does not apply to things that are part of a library or framework's programming style, like reversing a Django path name.

Magic numbers and strings

"Magic" numbers should be replaced by constants/variables. The same applies to "magic" string literals (e.g. a string where each character is used as a flag, or a function returning the name of a specific color).

Translations only differing in case

If there exists multiple translation strings with the only difference being the casing, they should ideally be replaced by the one with the "lowest" casing, and transformed after the translation has been invoked.

For example, if both "event" and "Event" exist as translation strings, the latter instance can be replaced by capfirst(_("event")) in Python code, and {% translate "event"|capfirst %} in template code.

Python

For code in general

Unused imports

Unused imports should be removed.

Unnecessary print statements

These should be removed, or replaced by fitting logging calls - preferably through the utility functions in logging_utils.py.

Variable, function or class names shadowing built-in or imported names

For example naming a parameter int, filter or range (shadowing built-in names), or date, time or path (shadowing imported names - if they're imported in the module in question).

A common way to avoid this is by adding e.g. an _ or _obj suffix, or by coming up with a different name altogether.

Exception clause is too broad

Having a bare except clause or simply catching Exception (or BaseException), should be avoided, as the appropriate reaction to an exception in most cases depends on the specific exception type.

Catching a too broad exception can be reasonable, however, if the caught exception is chained (using the from keyword) or if it's logged (in which case the exception object should be included).

Additional reasons and tips are summed up neatly in this article.

Using a model object's id instead of pk

The primary key field of a model can potentially be named something other than id, so the pk property should preferably always be used.

Missing prefetch of related objects

When iterating through querysets and using the value of the objects' relation fields (ForeignKey, OneToOneField and ManyToManyField), it's in most cases beneficial to prefetch the objects that the relation fields refer to, so that Django doesn't have to make an additional database query for each related object that the code uses the value of (this can naturally easily lead to considerable performance loss).
This is, in other words, mainly relevant for views displaying multiple objects.

This should be done by calling select_related() and/or prefetch_related() on the queryset - depending on the relation type of the related objects.

As an example, consider the following scenario, where the objects of two models - Event and EventOccurrence - are listed in a template:

from django.db import models


class Event(models.Model):
    pass


class EventOccurrence(models.Model):
    event = models.ForeignKey(
        to=Event,
        on_delete=models.CASCADE,
        related_name='occurrences',
    )


def get_context_data():
    # Prefetching related objects to avoid unnecessary additional database lookups
    return {
        'occurrence_objects': EventOccurrence.objects.select_related('event'),
        'event_objects': Event.objects.prefetch_related('occurrences'),
    }
<h2>Event occurrence list</h2>
<ul>
    {% for occurrence in occurrence_objects %}
        <li>
            {{ occurrence }}
            <!-- Here, the value of the related field `event` is used -->
            - {{ occurrence.event }}
        </li>
    {% endfor %}
</ul>

<h2>Nested event occurrence list</h2>
<ul>
    {% for event in event_objects %}
        <li>{{ event }}
            <ul>
                <!-- Here, the value of the related field `occurrences` (through the `related_name` argument) is iterated through -->
                {% for occurrence in event.occurrences %}
                    <li>{{ occurrence }}</li>
                {% endfor %}
            </ul>
        </li>
    {% endfor %}
</ul>

Unnecessary database queries

Attempt to minimize the number of unnecessary database queries as much as practically possible.

The number of database queries a request triggers, can be measured by adding the following code to the Django settings file:

from web.settings import LOGGING


LOGGING['loggers']['django.db.backends'] = {
    'level': 'DEBUG',
    'handlers': ['console'],
}
""" This would require a logging handler named `console`, which would look something like this:
'console': {
    'level': 'DEBUG',
    'filters': [...],
    'class': 'logging.StreamHandler',
}
"""

For each migration

RunPython operations missing the reverse_code argument

Custom-written migrations with RunPython operations should always provide the reverse_code argument, so that it's possible to unapply a migration.

If the RunPython operation doesn't need to be unapplied, or if it doesn't make sense to unapply it, one can pass migrations.RunPython.noop.

For each class

Fields declared outside of the standard places

All fields should be declared either at the top of the class body, or in the __init__() constructor - or in the setUp() method of a test class. A recommended way to declare fields that are not supposed to / don't need to have a default value, is to use the type hint syntax - without assigning a value (i.e. just <field name>: <field type>).

If creating a mixin class, and it would be unwieldy to declare all the fields that are set, one can instead add a # noinspection PyAttributeOutsideInit comment before the method or class definition.

For each model

Creating custom model permissions that perform the same role as one of Django's default permissions

Use one (or more) of Django's default permissions instead.

Missing __str__() method

It's often useful to be able to see a string representation of model objects - for example when debugging.

Missing get_absolute_url() method

If there exists a detail page for a specific object, the get_absolute_url() method should be defined and made to return the URL for that page - preferably a URL returned by django-hosts' reverse() function.

Missing custom admin class

It's generally useful for models to have a registered admin class, with properly customized fields (like list_display, list_filter and search_fields).

Making the model do validation

This includes doing validation in the save() method, in custom querysets' update() method, or in model signals.

Validation should generally only be done through forms. (This includes validators set in model fields' validators keyword argument - which are run in model forms - and models' validation/cleaning methods - which are also run in model forms.) Constraining models' data should generally only be done through the fields' arguments (like null or unique), or through model constraints.

A major reason for all this, is that it's useful for developers and admins to be able to change the state of objects to whatever they deem useful/valuable at the time - for example through the Django admin site, or through shell - even if it would not be valid for a user to set the same state through normal means on the website. Another reason is that saving or updating model objects is never expected to e.g. raise a ValidationError.

For each view

The view is a function

In almost all cases, views should be class-based, as this often decreases the amount of code required, and the potential for accidentally creating bugs (by e.g. forgetting something that the class-based views implement by default), and increases the readability and reusability.

Improper access control (permissions)

All views that do not present a public page, should extend PermissionRequiredMixin and set their permission_required field to one or more appropriate permissions - or override the has_permission() method, for more detailed control. Alternatively, they can extend LoginRequiredMixin or UserPassesTestMixin, if those are sufficient.

If a certain permission should only be enforced for a specific instance of a view, the as_view() call can be wrapped in one of the decorators corresponding to the mixin classes mentioned above (i.e. permission_required(), login_required(), and user_passes_test()). It's generally best to use our custom permission_required_else_denied() decorator, unless you're sure of otherwise.

Missing page title

Views responding with a complete webpage should always set the page title (i.e. the <title> tag), e.g. by setting the page_title context variable - which is used in web/base.html.

See the equivalent code smell for templates.

Modifying state in GET requests

This includes things like modifying objects in the database or files on the server.

Modifying state should be avoided to prevent things like browsers caching or prefetching the URL, or web crawlers regularly visiting the URL, and possibly prevent CSRF vulnerabilities (which would not be protected by Django's built-in CSRF protection, as that only takes effect for requests with "unsafe" HTTP methods).

Instead, implement the same functionality using a more proper HTTP method, like POST (CreateView), PUT (UpdateView) or DELETE (DeleteView).

Doing input validation directly in view code

This should almost always be done in a form - which is automatically done in views inheriting from ProcessFormView, like CreateView and UpdateView.

List view missing pagination

A view presenting a list of objects that has the potential to grow very long, should implement pagination - both so that it's easier to navigate, and so that it won't take an impractically long time to load the page.

Missing context_object_name

context_object_name should be explicitly defined - with a fitting name - so that it's easier to read and write the template code. This also implies that the default object and object_list template variables should preferably not be used.
(Naturally, this mainly applies to views inheriting from ListView or DetailView - or UpdateView if the object that's being updated is used in the template outside the form.)

For each form class

Missing empty field check

This applies especially to the clean() method, where field values should be looked up using dictionaries' get() method, and then checked for whether they're empty, before using/comparing them. For example:

from django import forms
from django.utils.translation import gettext_lazy as _


# (Extends `Form` for convenience; this should normally extend `ModelForm`)
class EventOccurrenceForm(forms.Form):
    start_time = forms.DateTimeField()
    end_time = forms.DateTimeField()

    def clean(self):
        cleaned_data = super().clean()
        # The fields won't exist in the cleaned data if they were submitted empty (even if they're both (by default) required)
        start_time = cleaned_data.get('start_time')
        end_time = cleaned_data.get('end_time')

        # These might be `None`
        if start_time and end_time:
            if start_time > end_time:
                error_message = _("The event cannot end before it starts.")
                code = 'invalid_relative_to_other_field'
                raise forms.ValidationError({
                    'start_time': forms.ValidationError(error_message, code=code),
                    'end_time': forms.ValidationError(error_message, code=code),
                })

        return cleaned_data

Missing error messages

When the form data is invalid in some way, raise one or more ValidationErrors (or call add_error()) with an appropriate message, which should make the error understandable to the user - including what the user can potentially do about the error (this can be implicit).

Improperly validating user input

Basic validation should preferably be done through validators set in the model fields' validators argument, or through the model's validation/cleaning methods.
(Be aware of the details and reasoning under Making the model do validation, though. Additionally, setting model field constraints can also be used in conjunction with the validation mentioned above, which is detailed under this related code smell.)

All other validation should normally be done in the form's clean() method and/or clean_<field name>() methods (see the steps for form and field validation).

For each test class

Test cases not cleaning up media files

When using temporary files (like test_utils.MOCK_JPG_FILE, which is a SimpleUploadedFile) in tests, these files should always be removed after the tests have run. This can be done by simply letting the test case class extend test_utils.CleanUpTempFilesTestMixin.

For each function/method

Mismatching overridden method signature

When overriding a method, the base method signature should in most cases be directly copied - including default parameter values and type hints. Misnamed parameters, or too few or too many parameters, makes it more challenging to familiarize oneself with the code, and can cause hard-to-discover bugs.

An exception to this, is "catching" parameters from the base method that are not used, in standard *args and **kwargs parameters.

Missing call to overridden method

If required, a method should always call the base method it overrides (i.e. super().<method name>()).

Missing returning required data

If required, a method - especially overridden methods - should always return the data expected by its caller. Examples include returning the cleaned data in a form's clean() method (or clean_<field name>() methods), or returning the created object in a form's save() method.

For each URL path

Missing name argument

The name keyword argument should always be set to an appropriate and unique name (see the docs for details on naming URL patterns).

Improper access control (permissions)

See the details of the equivalent code smell for views; moreover, always prefer doing access control in the views.

If multiple related paths should all have the same permission, they can be included using the decorator_include() decorator.

For each model field

Missing blank=True for non-required model fields

Model fields that are not required to be specified when creating/updating objects (e.g. if they have null=True), should have their blank keyword argument set to True, so that model forms set their required attribute accordingly.

String-based model fields with null=True

Excerpt from the docs on the null option:

Avoid using null on string-based fields such as CharField and TextField. If a string-based field has null=True, that means it has two possible values for “no data”: NULL, and the empty string.

String-based model fields with unnecessary max_length

The max_length keyword argument should rarely be set - unless if it would break things to not always enforce a length limit; simply preventing users from submitting a ludicrously long string should instead be done by the forms.

An exception is for fields with the choices keyword argument set to a collection of strings that are all strictly of a certain length (or shorter), where max_length could be set to this length.

Missing related_name for relation fields

Relation fields (ForeignKey, OneToOneField and ManyToManyField) should always be passed the related_name keyword argument - and it should be set to a sensible name.

Missing verbose_name

The verbose_name keyword argument should always be set to a translated string.

Missing appropriate constraint on field or combination of fields

This should ideally be done by going through each of the model fields' options (like null, unique or max_length) and considering whether they're appropriately set (or not set, if the default value is suitable), and by reviewing the existing model constraints and considering whether they're sufficient, or if some should be added. The latter can be used to e.g. ensure the value of a field is unique per value of another field, among other things.

(While doing this, be aware of the details and reasoning under Making the model do validation.)

Django templates / HTML / CSS

For code in general

Lacking accessibility

Mozilla's accessibility testing checklist includes a useful overview over things that can be checked that are relevant to this code smell. A more complete and detailed "checklist" is the Web Content Accessibility Guidelines (WCAG) standard, which provides a customizable quick reference - including choosing between three levels of conformance to the standard.

To implement proper accessibility practices, Mozilla has a must-read introductory guide on HTML and accessibility, and a guide on WAI-ARIA implementations - with slightly more advanced topics, which can further improve the user experience.

Some easily accessible tools that can be used in addition to the ones mentioned in the checklist above, include using Firefox's Accessibility Inspector to check the outline of the page, and using Chrome's Lighthouse feature to audit a page's technical accessibility aspects - in addition to several other aspects of the page's quality.

Lastly, to quote Mozilla's HTML and accessibility guide mentioned above:

The goal isn't "all or nothing"; every improvement you can make will help the cause of accessibility.

Using a model object's id instead of pk

See the equivalent code smell for Python.

Missing page title

Templates rendering a complete webpage should always set the page title (i.e. the <title> tag).

If the template is extending web/base.html (directly or through another template), this can be done e.g. by setting the page_title context variable or by overriding the title block.

See the equivalent code smell for views.

Page flow relying too much on JavaScript

Django excels particularly at facilitating creating static pages, and so it's generally good practice to utilize that strength, as it will very often make the code considerably smaller (due to the large amount of generic code that is included with Django), easier to read and maintain, and easier to link to specific states of a page (by implementing URL query parameters). The Django admin site contains multiple good examples of this, like:

  • A separate page for editing and displaying the details of a specific model object - in contrast to e.g. a modal
  • Doing searching, filtering, sorting and pagination through URL query parameters
  • Doing various actions (like deletion) on one or more model objects at a time using a bound form, which also shows details and consequences of completing those actions

Of course, the decision on whether to make a specific page more static or more dynamic, should be made on a case-by-case basis, and by taking both user experience and code maintainability into consideration.

CSS directly in HTML

Inline CSS (in HTML tags' style attribute) and CSS in <style> tags, should be moved to stylesheets (.css files) and linked with:

<link rel="stylesheet" href="{% static '<!-- path -->' %}"/>

For each HTML tag

<link> or <script> tags in a template that's included elsewhere

These should be moved to the <head> tag of the templates that include this template, so that the files linked are not potentially linked multiple times. Furthermore, it's good practice to add a comment in the original template, saying which files should be linked when include-ing the template.

Links missing target="_blank"

<a> tags leading the user away from the "flow" of the current page, should always have its target attribute set to "_blank". This will make the link open in a new tab when clicked.

For each HTML attribute

Unused/unnecessary attribute

These should be removed.

Unused class

Classes of a class attribute that have no effect, should be removed.

For each CSS rule

Unused CSS rules

These should be removed. This includes rules selecting classes or IDs that do not exist (anymore).

Unnecessary CSS properties

CSS properties that have no effect, should be removed. For example, setting the left property on an element that has position: static.

JavaScript

For code in general

JavaScript directly in HTML

JavaScript code written within <script> tags, should be moved to .js files and linked with:

<script <!-- execution attribute --> src="{% static '<!-- path -->' %}"></script>

where <!-- execution attribute --> can be defer or async - if appropriate.

If the code needs some values from template variables, the values can be stored in var variables in a <script> tag - which should also be declared at the top of the .js file(s) they're used in. In that case, it's good practice to include a comment saying where the values of these variables come from.

Making code wait until the document is ready / window has loaded

This includes writing code within functions like $(document).ready(function () {}) or (function() {})(), and should be replaced by linking the code in a <script> tag with either the defer or async attribute present.

Missing semicolon

All statements should end with a ;.

Comparing values loosely (with ==)

The === operator should always be favored over ==. See some of the reasons in the answers to this question.

Of course, this also applies to !== vs. !=.

Unnecessary console.log() statements

See the equivalent code smell for Python.