Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow BooleanField(null=True) on Django >= 2.1 #38

Closed
wants to merge 5 commits into from
Closed

Allow BooleanField(null=True) on Django >= 2.1 #38

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c9a27da
Allow BooleanField(null=True) on Django >= 2.1
washeck May 3, 2019
1695845
Add Django to tox dependencies
washeck Nov 22, 2019
abc0956
Run coverage test from tox
washeck Nov 22, 2019
686cba5
Revert unintended change in setup.py
washeck Nov 22, 2019
afcb5b4
Configure pytest-cov to append results
washeck Nov 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,6 @@ jobs:
command: |
. venv/bin/activate
git diff HEAD $(git merge-base HEAD origin/master) | flake8 --diff
- run:
name: run coverage
command: |
. venv/bin/activate
pip install -e .
py.test --cov=. --cov-fail-under=100
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of removing this code, let's create a requirements/test.txt and add Django as a requirements for tests.

- run:
name: upload coverage report
command: |
Expand Down
13 changes: 9 additions & 4 deletions flake8_django/checkers/model_fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
import django
from .checker import Checker
from .issue import Issue

ALLOW_NULL_BOOLEAN = django.VERSION >= (2, 1)

NOT_NULL_TRUE_FIELDS = [
'CharField', 'TextField', 'SlugField',
'EmailField', 'UUIDField', 'ImageField',
'FileField', 'FilePathField', 'URLField'
'CharField', 'TextField', 'SlugField', 'EmailField', 'Field',
'UUIDField', 'ImageField', 'FileField'
]
NOT_BLANK_TRUE_FIELDS = ['BooleanField']
NOT_BLANK_TRUE_FIELDS = []
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests are failing now, because coverage decreases from 100% to 99% due to tests on test_model_fields where NOT_BLANK_TRUE_FIELDS is uses as a parameter. Since the list is empty this tests are not running, so that part of the code is not being covered.

I think, let's remove NOT_BLANK_TRUE_FIELDS altogether, since we could it's ok to have BooleanField with blank=True even in older version, that's just going to make the system use the default value.

What are your thoughts?


if not ALLOW_NULL_BOOLEAN:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do the check for django.VERSION here? Just so that we make it more explicit?

NOT_NULL_TRUE_FIELDS.append('BooleanField')
NOT_BLANK_TRUE_FIELDS.append('BooleanField')


class DJ01(Issue):
Expand Down
35 changes: 32 additions & 3 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,38 @@
[tox]
envlist = py34,py35,py36,py37
envlist =
clean
django22-py{37,36}
django21-py{37,36,35}
django20-py{36,35,34}
report
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove clean and report from the envlist. We don't want to run coverage for every environment.


[flake8]
max-line-length = 120
exclude = tests/fixtures/*

[testenv]
deps = pytest
commands =
deps =
pytest
pytest-cov
django20: {[django]2.0.x}
django21: {[django]2.1.x}
django22: {[django]2.2.x}

commands =
py.test --cov=./flake8_django --cov-append

[testenv:clean]
deps = coverage
skip_install = true
commands = coverage erase
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This clean part is not needed.


[testenv:report]
skip_install = true
deps = coverage
commands =
coverage report --fail-under=100
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This report part is not needed, since coverage reports are being uploaded to codecov.


[django]
2.0.x = Django>=2.0,<2.1
2.1.x = Django>=2.1,<2.2
2.2.x = Django>=2.2,<2.3