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.

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

Add ruff rules A,B,C4,EM,ISC,PERF,PGH,RET,RUF,S,SIM,SLF #549

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@ repos:
- id: trailing-whitespace

- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.8.4
rev: v0.9.7
hooks:
- id: ruff
2 changes: 1 addition & 1 deletion docs/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@

# General information about the project.
project = 'django-waffle'
copyright = '2012-2018, James Socol'
copyright = '2012-2018, James Socol' # noqa: A001

# The version info for the project you're documenting, acts as replacement for
# |version| and |release|, also used in various other places throughout the
Expand Down
34 changes: 20 additions & 14 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -71,26 +71,38 @@ target-version = "py38"

[tool.ruff.lint]
select = [
"A", # flake8-builtins
"AIR", # Airflow
"ASYNC", # flake8-async
"B", # flake8-bugbear
"BLE", # flake8-blind-except
"C4", # flake8-comprehensions
"C90", # McCabe cyclomatic complexity
"DJ", # flake8-django
"DTZ", # flake8-datetimez
"E", # pycodestyle errors
"EM", # flake8-errmsg
"F", # Pyflakes
"FIX", # flake8-fixme
"FLY", # flynt
"G", # flake8-logging-format
"ICN", # flake8-import-conventions
"INP", # flake8-no-pep420
"INT", # flake8-gettext
"ISC", # flake8-implicit-str-concat
Copy link
Contributor

Choose a reason for hiding this comment

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

The above lints look useful to me in that the changes made to make the lints pass mostly look like useful improvements. The possible exception to my mind is defining a msg variable before raising the exception - I am ambivalent to that one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

% ruff rule EM101

Why is this bad?
By using a string literal, the error message will be duplicated in the traceback, which can make the traceback less readable.

If you would like, we can revert the EM transformations.

% ruff check --select=EM--statistics

10	EM101  	[*] raw-string-in-exception
 6	EM102  	[*] f-string-in-exception
[*] fixable with `ruff check --fix`

"NPY", # NumPy-specific rules
"PD", # pandas-vet
"PERF", # Perflint
Copy link
Contributor

Choose a reason for hiding this comment

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

perflint describes itself as "This project is an early beta. It will likely raise many false-positives in your code" - you have previously said that you think any and all passing lints should be included - more is necessarily better.
I am worried that new linting raises the bar for new contributors, making it harder to get started - linting has advantages and disadvantages.
If any lints should be excluded then I think this one should, given the warning from its creator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pull request did not make any PERF-related changes (other than enabling PERF).

When I read the Why is this bad? section of these rule I see quantified performance gains.

Here we are not running perflint. Instead, we are running ruff which uses the original project as inspiration but rewrites each rule that the project team deems valuable in Rust. Often useless or conflicting rules are dropped. There is active discussion in the ruff repo about which rules to adopt and which to drop.

If you would like, we can disable the PERF rules.

"PGH", # pygrep-hooks
"PIE", # flake8-pie
"PL", # Pylint
"PYI", # flake8-pyi
"RET", # flake8-return
"RSE", # flake8-raise
"RUF", # Ruff-specific rules
"S", # flake8-bandit
"SIM", # flake8-simplify
"SLF", # flake8-self
"SLOT", # flake8-slots
"T10", # flake8-debugger
"T20", # flake8-print
Expand All @@ -99,32 +111,20 @@ select = [
"UP", # pyupgrade
"W", # pycodestyle warnings
"YTT", # flake8-2020
# "A", # flake8-builtins
# "ANN", # flake8-annotations
# "ARG", # flake8-unused-arguments
# "B", # flake8-bugbear
# "C4", # flake8-comprehensions
# "COM", # flake8-commas
# "CPY", # flake8-copyright
# "CPY", # flake8-copyright
# "D", # pydocstyle
# "EM", # flake8-errmsg
# "ERA", # eradicate
# "EXE", # flake8-executable
# "FA", # flake8-future-annotations
# "FBT", # flake8-boolean-trap
# "I", # isort
# "ISC", # flake8-implicit-str-concat
# "N", # pep8-naming
# "PERF", # Perflint
# "PGH", # pygrep-hooks
# "PT", # flake8-pytest-style
# "PTH", # flake8-use-pathlib
# "Q", # flake8-quotes
# "RET", # flake8-return
# "RUF", # Ruff-specific rules
# "S", # flake8-bandit
# "SIM", # flake8-simplify
# "SLF", # flake8-self
# "TCH", # flake8-type-checking
# "TRY", # tryceratops
]
Expand All @@ -135,14 +135,20 @@ exclude = [
"*/migrations/*",
"docs",
]
ignore = ["F401"]
ignore = [
"F401", # Imported but unused
"ISC002", # Implicit string concatenation may conflict with black and ruff format
"S101", # Use of assert detected
]

[tool.ruff.lint.mccabe]
max-complexity = 23

[tool.ruff.lint.per-file-ignores]
"docs/conf.py" = ["INP001"]
"test_app/models.py" = ["DJ008"] # FIXME
"waffle/admin.py" = ["SLF001"] # FIXME
"waffle/jinja.py" = [ "PGH003" ] # FIXME
"waffle/models.py" = ["DJ012", "PYI019"] # FIXME

[tool.ruff.lint.pylint]
Expand Down
2 changes: 2 additions & 0 deletions test_app/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ def is_active_for_user(self, user):
company_ids = self._get_company_ids()
if user.company_id in company_ids:
return True
return None
return None

def _get_company_ids(self):
cache_key = keyfmt(
Expand Down
2 changes: 1 addition & 1 deletion test_settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
SITE_ID = 1
USE_I18N = False

SECRET_KEY = 'foobar'
SECRET_KEY = 'foobar' # noqa: S105

DATABASES = {
'default': {
Expand Down
12 changes: 6 additions & 6 deletions waffle/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,9 @@ def get_waffle_model(setting_name: str) -> (

try:
return django_apps.get_model(flag_model_name)
except ValueError:
raise ImproperlyConfigured(f"WAFFLE_{setting_name} must be of the form 'app_label.model_name'")
except LookupError:
raise ImproperlyConfigured(
f"WAFFLE_{setting_name} refers to model '{flag_model_name}' that has not been installed"
)
except ValueError as ve:
msg = f"WAFFLE_{setting_name} must be of the form 'app_label.model_name'"
raise ImproperlyConfigured(msg) from ve
except LookupError as le:
msg = f"WAFFLE_{setting_name} refers to model '{flag_model_name}' that has not been installed"
raise ImproperlyConfigured(msg) from le
10 changes: 5 additions & 5 deletions waffle/admin.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from __future__ import annotations

from typing import Any
from typing import Any, ClassVar

from django.contrib import admin
from django.contrib.admin.models import LogEntry, CHANGE, DELETION
Expand Down Expand Up @@ -84,13 +84,13 @@ def label_and_url_for_value(self, values: Any) -> tuple[str, str]:
.using(self.db) \
.get(**{key: value})
names.append(escape(str(name)))
except self.rel.model.DoesNotExist:
except self.rel.model.DoesNotExist: # noqa: PERF203
names.append('<missing>')
return "(" + ", ".join(names) + ")", ""


class FlagAdmin(BaseAdmin):
actions = [enable_for_all, disable_for_all, delete_individually]
actions: ClassVar = [enable_for_all, disable_for_all, delete_individually]
list_display = ('name', 'note', 'everyone', 'percent', 'superusers',
'staff', 'authenticated', 'languages')
list_filter = ('everyone', 'superusers', 'staff', 'authenticated')
Expand Down Expand Up @@ -131,14 +131,14 @@ def disable_switches(ma, request, qs):


class SwitchAdmin(BaseAdmin):
actions = [enable_switches, disable_switches, delete_individually]
actions: ClassVar = [enable_switches, disable_switches, delete_individually]
list_display = ('name', 'active', 'note', 'created', 'modified')
list_filter = ('active',)
ordering = ('-id',)


class SampleAdmin(BaseAdmin):
actions = [delete_individually]
actions: ClassVar = [delete_individually]
list_display = ('name', 'percent', 'note', 'created', 'modified')
ordering = ('-id',)

Expand Down
2 changes: 1 addition & 1 deletion waffle/apps.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@ class WaffleConfig(AppConfig):
default_auto_field = 'django.db.models.AutoField'

def ready(self) -> None:
import waffle.signals # noqa: F401
import waffle.signals
6 changes: 2 additions & 4 deletions waffle/decorators.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ def _wrapped_view(request, *args, **kwargs):
response_to_redirect_to = get_response_to_redirect(redirect_to, *args, **kwargs)
if response_to_redirect_to:
return response_to_redirect_to
else:
raise Http404
raise Http404

return view(request, *args, **kwargs)
return _wrapped_view
Expand All @@ -48,8 +47,7 @@ def _wrapped_view(request, *args, **kwargs):
response_to_redirect_to = get_response_to_redirect(redirect_to, *args, **kwargs)
if response_to_redirect_to:
return response_to_redirect_to
else:
raise Http404
raise Http404

return view(request, *args, **kwargs)
return _wrapped_view
Expand Down
22 changes: 13 additions & 9 deletions waffle/management/commands/waffle_flag.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,14 +61,14 @@ def add_arguments(self, parser: CommandParser) -> None:
parser.add_argument(
'--group', '-g',
action='append',
default=list(),
default=[],
help='Turn on the flag for listed group names (use flag more '
'than once for multiple groups). WARNING: This will remove '
'any currently associated groups unless --append is used!')
parser.add_argument(
'--user', '-u',
action='append',
default=list(),
default=[],
help='Turn on the flag for listed usernames (use flag more '
'than once for multiple users). WARNING: This will remove '
'any currently associated users unless --append is used!')
Expand Down Expand Up @@ -111,7 +111,8 @@ def handle(self, *args: Any, **options: Any) -> None:
flag_name = options['name']

if not flag_name:
raise CommandError('You need to specify a flag name.')
msg = 'You need to specify a flag name.'
raise CommandError(msg)

if options['create']:
flag, created = get_waffle_flag_model().objects.get_or_create(name=flag_name)
Expand All @@ -120,8 +121,9 @@ def handle(self, *args: Any, **options: Any) -> None:
else:
try:
flag = get_waffle_flag_model().objects.get(name=flag_name)
except get_waffle_flag_model().DoesNotExist:
raise CommandError('This flag does not exist.')
except get_waffle_flag_model().DoesNotExist as dne:
msg = 'This flag does not exist.'
raise CommandError(msg) from dne

# Group isn't an attribute on the Flag, but a related Many-to-Many
# field, so we handle it a bit differently by looking up groups and
Expand All @@ -133,8 +135,9 @@ def handle(self, *args: Any, **options: Any) -> None:
try:
group_instance = Group.objects.get(name=group)
group_hash[group_instance.name] = group_instance.id
except Group.DoesNotExist:
raise CommandError(f'Group {group} does not exist')
except Group.DoesNotExist as dne: # noqa: PERF203
msg = f'Group {group} does not exist'
raise CommandError(msg) from dne
# If 'append' was not passed, we clear related groups
if not options_append:
flag.groups.clear()
Expand All @@ -152,8 +155,9 @@ def handle(self, *args: Any, **options: Any) -> None:
| Q(**{UserModel.EMAIL_FIELD: username})
)
user_hash.add(user_instance)
except UserModel.DoesNotExist:
raise CommandError(f'User {username} does not exist')
except UserModel.DoesNotExist as dne: # noqa: PERF203
msg = f'User {username} does not exist'
raise CommandError(msg) from dne
# If 'append' was not passed, we clear related users
if not options_append:
flag.users.clear()
Expand Down
15 changes: 8 additions & 7 deletions waffle/management/commands/waffle_sample.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,16 @@ def handle(self, *args: Any, **options: Any) -> None:
percent = options['percent']

if not (sample_name and percent):
raise CommandError(
'You need to specify a sample name and percentage.'
)
msg = 'You need to specify a sample name and percentage.'
raise CommandError(msg)

try:
percent = float(percent)
if not (0.0 <= percent <= 100.0):
raise ValueError
except ValueError:
raise CommandError('You need to enter a valid percentage value.')
except ValueError as ve:
msg = 'You need to enter a valid percentage value.'
raise CommandError(msg) from ve

if options['create']:
sample, created = get_waffle_sample_model().objects.get_or_create(
Expand All @@ -60,8 +60,9 @@ def handle(self, *args: Any, **options: Any) -> None:
else:
try:
sample = get_waffle_sample_model().objects.get(name=sample_name)
except get_waffle_sample_model().DoesNotExist:
raise CommandError('This sample does not exist.')
except get_waffle_sample_model().DoesNotExist as dne:
msg = 'This sample does not exist.'
raise CommandError(msg) from dne

sample.percent = percent
sample.save()
14 changes: 8 additions & 6 deletions waffle/management/commands/waffle_switch.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@


def on_off_bool(string: str) -> bool:
if string not in ['on', 'off']:
raise ArgumentTypeError(f"invalid choice: {string!r} (choose from 'on', "
"'off')")
if string not in {'on', 'off'}:
msg = f"invalid choice: {string!r} (choose from 'on', 'off')"
raise ArgumentTypeError(msg)
return string == 'on'


Expand Down Expand Up @@ -53,7 +53,8 @@ def handle(self, *args: Any, **options: Any) -> None:
state = options['state']

if not (switch_name and state is not None):
raise CommandError('You need to specify a switch name and state.')
msg = 'You need to specify a switch name and state.'
raise CommandError(msg)

if options["create"]:
switch, created = get_waffle_switch_model().objects.get_or_create(
Expand All @@ -64,8 +65,9 @@ def handle(self, *args: Any, **options: Any) -> None:
else:
try:
switch = get_waffle_switch_model().objects.get(name=switch_name)
except get_waffle_switch_model().DoesNotExist:
raise CommandError('This switch does not exist.')
except get_waffle_switch_model().DoesNotExist as dne:
msg = 'This switch does not exist.'
raise CommandError(msg) from dne

switch.active = state
switch.save()
2 changes: 1 addition & 1 deletion waffle/managers.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@


if TYPE_CHECKING:
from waffle.models import _BaseModelType, AbstractBaseFlag, AbstractBaseSample, AbstractBaseSwitch # noqa: F401
from waffle.models import _BaseModelType, AbstractBaseFlag, AbstractBaseSample, AbstractBaseSwitch
else:
_BaseModelType = TypeVar("_BaseModelType")

Expand Down
7 changes: 2 additions & 5 deletions waffle/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,8 @@ def process_response(self, request: HttpRequest, response: HttpResponse) -> Http
for k in request.waffles:
name = smart_str(get_setting('COOKIE') % k)
active, rollout = request.waffles[k]
if rollout and not active:
# "Inactive" is a session cookie during rollout mode.
age = None
else:
age = max_age
# "Inactive" is a session cookie during rollout mode.
age = None if rollout and not active else max_age
response.set_cookie(name, value=active, max_age=age,
secure=secure)
if hasattr(request, 'waffle_tests'):
Expand Down
9 changes: 4 additions & 5 deletions waffle/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,12 @@ class BaseWaffleMixin:

def validate_waffle(self, waffle, func):
if waffle.startswith('!'):
active = not func(waffle[1:])
else:
active = func(waffle)
return active
return not func(waffle[1:])
return func(waffle)

def invalid_waffle(self):
raise Http404('Inactive waffle')
msg = 'Inactive waffle'
raise Http404(msg)


class WaffleFlagMixin(BaseWaffleMixin):
Expand Down
Loading
Loading