Skip to content

Commit

Permalink
Closes #17733: Replace pycodestyle with ruff (#17734)
Browse files Browse the repository at this point in the history
* Resolve F541 errors

* Resolve F841 errors

* Resolve F811 errors

* Resolve F901 errors

* Resolve E714 errors

* Ignore F821 errors for GraphQL mixins

* Replace pycodestyle with ruff

* Move ignores to ruff.toml
  • Loading branch information
jeremystretch authored and bctiemann committed Oct 11, 2024
1 parent ed8f370 commit ee15a57
Show file tree
Hide file tree
Showing 65 changed files with 166 additions and 197 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ jobs:
run: |
python -m pip install --upgrade pip
pip install -r requirements.txt
pip install pycodestyle coverage tblib
pip install ruff coverage tblib
- name: Build documentation
run: mkdocs build
Expand All @@ -85,7 +85,7 @@ jobs:
run: python netbox/manage.py makemigrations --check

- name: Check PEP8 compliance
run: pycodestyle --ignore=W504,E501 --exclude=node_modules netbox/
run: ruff check netbox/

- name: Check UI ESLint, TypeScript, and Prettier Compliance
run: yarn --cwd netbox/project-static validate
Expand Down
4 changes: 2 additions & 2 deletions docs/development/getting-started.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,10 @@ NetBox ships with a [git pre-commit hook](https://githooks.com/) script that aut
cd .git/hooks/
ln -s ../../scripts/git-hooks/pre-commit
```
For the pre-commit hooks to work, you will also need to install the pycodestyle package:
For the pre-commit hooks to work, you will also need to install the [ruff](https://docs.astral.sh/ruff/) linter:

```no-highlight
python -m pip install pycodestyle
python -m pip install ruff
```
...and set up the yarn packages as shown in the [Web UI Development Guide](web-ui.md)

Expand Down
32 changes: 16 additions & 16 deletions docs/development/style-guide.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Style Guide

NetBox generally follows the [Django style guide](https://docs.djangoproject.com/en/stable/internals/contributing/writing-code/coding-style/), which is itself based on [PEP 8](https://www.python.org/dev/peps/pep-0008/). [Pycodestyle](https://github.com/pycqa/pycodestyle) is used to validate code formatting, ignoring certain violations.
NetBox generally follows the [Django style guide](https://docs.djangoproject.com/en/stable/internals/contributing/writing-code/coding-style/), which is itself based on [PEP 8](https://www.python.org/dev/peps/pep-0008/). [ruff](https://docs.astral.sh/ruff/) is used for linting (with certain [exceptions](#linter-exceptions)).

## Code

Expand All @@ -20,32 +20,32 @@ NetBox generally follows the [Django style guide](https://docs.djangoproject.com

* Nested API serializers generate minimal representations of an object. These are stored separately from the primary serializers to avoid circular dependencies. Always import nested serializers from other apps directly. For example, from within the DCIM app you would write `from ipam.api.nested_serializers import NestedIPAddressSerializer`.

### PEP 8 Exceptions
### Linting

NetBox ignores certain PEP8 assertions. These are listed below.
The [ruff](https://docs.astral.sh/ruff/) linter is used to enforce code style. A [pre-commit hook](./getting-started.md#3-enable-pre-commit-hooks) which runs this automatically is included with NetBox. To invoke `ruff` manually, run:

#### Wildcard Imports
```
ruff check netbox/
```

Wildcard imports (for example, `from .constants import *`) are acceptable under any of the following conditions:
#### Linter Exceptions

* The library being import contains only constant declarations (e.g. `constants.py`)
* The library being imported explicitly defines `__all__`
The following rules are ignored when linting.

#### Maximum Line Length (E501)
##### [E501](https://docs.astral.sh/ruff/rules/line-too-long/): Line too long

NetBox does not restrict lines to a maximum length of 79 characters. We use a maximum line length of 120 characters, however this is not enforced by CI. The maximum length does not apply to HTML templates or to automatically generated code (e.g. database migrations).
NetBox does not enforce a hard restriction on line length, although a maximum length of 120 characters is strongly encouraged for Python code where possible. The maximum length does not apply to HTML templates or to automatically generated code (e.g. database migrations).

#### Line Breaks Following Binary Operators (W504)
##### [F403](https://docs.astral.sh/ruff/rules/undefined-local-with-import-star/): Undefined local with import star

Line breaks are permitted following binary operators.
Wildcard imports (for example, `from .constants import *`) are acceptable under any of the following conditions:

### Enforcing Code Style
* The library being import contains only constant declarations (e.g. `constants.py`)
* The library being imported explicitly defines `__all__`

The [`pycodestyle`](https://pypi.org/project/pycodestyle/) utility (formerly `pep8`) is used by the CI process to enforce code style. A [pre-commit hook](./getting-started.md#3-enable-pre-commit-hooks) which runs this automatically is included with NetBox. To invoke `pycodestyle` manually, run:
##### [F405](https://docs.astral.sh/ruff/rules/undefined-local-with-import-star-usage/): Undefined local with import star usage

```
pycodestyle --ignore=W504,E501 netbox/
```
The justification for ignoring this rule is the same as F403 above.

### Introducing New Dependencies

Expand Down
2 changes: 1 addition & 1 deletion netbox/circuits/api/nested_serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

# TODO: Remove in v4.2
warnings.warn(
f"Dedicated nested serializers will be removed in NetBox v4.2. Use Serializer(nested=True) instead.",
"Dedicated nested serializers will be removed in NetBox v4.2. Use Serializer(nested=True) instead.",
DeprecationWarning
)

Expand Down
2 changes: 1 addition & 1 deletion netbox/circuits/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ def setUpTestData(cls):
)

cls.csv_update_data = (
f"id,cid,description,status",
"id,cid,description,status",
f"{circuits[0].pk},Circuit 7,New description7,{CircuitStatusChoices.STATUS_DECOMMISSIONED}",
f"{circuits[1].pk},Circuit 8,New description8,{CircuitStatusChoices.STATUS_DECOMMISSIONED}",
f"{circuits[2].pk},Circuit 9,New description9,{CircuitStatusChoices.STATUS_DECOMMISSIONED}",
Expand Down
2 changes: 1 addition & 1 deletion netbox/core/api/nested_serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

# TODO: Remove in v4.2
warnings.warn(
f"Dedicated nested serializers will be removed in NetBox v4.2. Use Serializer(nested=True) instead.",
"Dedicated nested serializers will be removed in NetBox v4.2. Use Serializer(nested=True) instead.",
DeprecationWarning
)

Expand Down
2 changes: 1 addition & 1 deletion netbox/core/data_backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class LocalBackend(DataBackend):

@contextmanager
def fetch(self):
logger.debug(f"Data source type is local; skipping fetch")
logger.debug("Data source type is local; skipping fetch")
local_path = urlparse(self.url).path # Strip file:// scheme

yield local_path
Expand Down
2 changes: 1 addition & 1 deletion netbox/core/graphql/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
class ChangelogMixin:

@strawberry_django.field
def changelog(self, info) -> List[Annotated["ObjectChangeType", strawberry.lazy('.types')]]:
def changelog(self, info) -> List[Annotated["ObjectChangeType", strawberry.lazy('.types')]]: # noqa: F821
content_type = ContentType.objects.get_for_model(self)
object_changes = ObjectChange.objects.filter(
changed_object_type=content_type,
Expand Down
4 changes: 2 additions & 2 deletions netbox/core/management/commands/syncdatasource.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def handle(self, *args, **options):
if invalid_names := set(options['name']) - found_names:
raise CommandError(f"Invalid data source names: {', '.join(invalid_names)}")
else:
raise CommandError(f"Must specify at least one data source, or set --all.")
raise CommandError("Must specify at least one data source, or set --all.")

if len(options['name']) > 1:
self.stdout.write(f"Syncing {len(datasources)} data sources.")
Expand All @@ -43,4 +43,4 @@ def handle(self, *args, **options):
raise e

if len(options['name']) > 1:
self.stdout.write(f"Finished.")
self.stdout.write("Finished.")
2 changes: 1 addition & 1 deletion netbox/core/models/data.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ def clean(self):
# Ensure URL scheme matches selected type
if self.backend_class.is_local and self.url_scheme not in ('file', ''):
raise ValidationError({
'source_url': f"URLs for local sources must start with file:// (or specify no scheme)"
'source_url': "URLs for local sources must start with file:// (or specify no scheme)"
})

def to_objectchange(self, action):
Expand Down
4 changes: 2 additions & 2 deletions netbox/core/models/jobs.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,9 @@ def get_absolute_url(self):
# TODO: Employ dynamic registration
if self.object_type:
if self.object_type.model == 'reportmodule':
return reverse(f'extras:report_result', kwargs={'job_pk': self.pk})
return reverse('extras:report_result', kwargs={'job_pk': self.pk})
elif self.object_type.model == 'scriptmodule':
return reverse(f'extras:script_result', kwargs={'job_pk': self.pk})
return reverse('extras:script_result', kwargs={'job_pk': self.pk})
return reverse('core:job', args=[self.pk])

def get_status_color(self):
Expand Down
2 changes: 1 addition & 1 deletion netbox/dcim/api/nested_serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@

# TODO: Remove in v4.2
warnings.warn(
f"Dedicated nested serializers will be removed in NetBox v4.2. Use Serializer(nested=True) instead.",
"Dedicated nested serializers will be removed in NetBox v4.2. Use Serializer(nested=True) instead.",
DeprecationWarning
)

Expand Down
4 changes: 2 additions & 2 deletions netbox/dcim/forms/object_create.py
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,8 @@ class FrontPortCreateForm(ComponentCreateForm, model_forms.FrontPortForm):
# TODO: Clean up the application of HTMXSelect attributes
attrs={
'hx-get': '.',
'hx-include': f'#form_fields',
'hx-target': f'#form_fields',
'hx-include': '#form_fields',
'hx-target': '#form_fields',
}
)
)
Expand Down
40 changes: 20 additions & 20 deletions netbox/dcim/graphql/mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,33 +10,33 @@

@strawberry.type
class CabledObjectMixin:
cable: Annotated["CableType", strawberry.lazy('dcim.graphql.types')] | None
cable: Annotated["CableType", strawberry.lazy('dcim.graphql.types')] | None # noqa: F821

link_peers: List[Annotated[Union[
Annotated["CircuitTerminationType", strawberry.lazy('circuits.graphql.types')],
Annotated["ConsolePortType", strawberry.lazy('dcim.graphql.types')],
Annotated["ConsoleServerPortType", strawberry.lazy('dcim.graphql.types')],
Annotated["FrontPortType", strawberry.lazy('dcim.graphql.types')],
Annotated["InterfaceType", strawberry.lazy('dcim.graphql.types')],
Annotated["PowerFeedType", strawberry.lazy('dcim.graphql.types')],
Annotated["PowerOutletType", strawberry.lazy('dcim.graphql.types')],
Annotated["PowerPortType", strawberry.lazy('dcim.graphql.types')],
Annotated["RearPortType", strawberry.lazy('dcim.graphql.types')],
Annotated["CircuitTerminationType", strawberry.lazy('circuits.graphql.types')], # noqa: F821
Annotated["ConsolePortType", strawberry.lazy('dcim.graphql.types')], # noqa: F821
Annotated["ConsoleServerPortType", strawberry.lazy('dcim.graphql.types')], # noqa: F821
Annotated["FrontPortType", strawberry.lazy('dcim.graphql.types')], # noqa: F821
Annotated["InterfaceType", strawberry.lazy('dcim.graphql.types')], # noqa: F821
Annotated["PowerFeedType", strawberry.lazy('dcim.graphql.types')], # noqa: F821
Annotated["PowerOutletType", strawberry.lazy('dcim.graphql.types')], # noqa: F821
Annotated["PowerPortType", strawberry.lazy('dcim.graphql.types')], # noqa: F821
Annotated["RearPortType", strawberry.lazy('dcim.graphql.types')], # noqa: F821
], strawberry.union("LinkPeerType")]]


@strawberry.type
class PathEndpointMixin:

connected_endpoints: List[Annotated[Union[
Annotated["CircuitTerminationType", strawberry.lazy('circuits.graphql.types')],
Annotated["ConsolePortType", strawberry.lazy('dcim.graphql.types')],
Annotated["ConsoleServerPortType", strawberry.lazy('dcim.graphql.types')],
Annotated["FrontPortType", strawberry.lazy('dcim.graphql.types')],
Annotated["InterfaceType", strawberry.lazy('dcim.graphql.types')],
Annotated["PowerFeedType", strawberry.lazy('dcim.graphql.types')],
Annotated["PowerOutletType", strawberry.lazy('dcim.graphql.types')],
Annotated["PowerPortType", strawberry.lazy('dcim.graphql.types')],
Annotated["ProviderNetworkType", strawberry.lazy('circuits.graphql.types')],
Annotated["RearPortType", strawberry.lazy('dcim.graphql.types')],
Annotated["CircuitTerminationType", strawberry.lazy('circuits.graphql.types')], # noqa: F821
Annotated["ConsolePortType", strawberry.lazy('dcim.graphql.types')], # noqa: F821
Annotated["ConsoleServerPortType", strawberry.lazy('dcim.graphql.types')], # noqa: F821
Annotated["FrontPortType", strawberry.lazy('dcim.graphql.types')], # noqa: F821
Annotated["InterfaceType", strawberry.lazy('dcim.graphql.types')], # noqa: F821
Annotated["PowerFeedType", strawberry.lazy('dcim.graphql.types')], # noqa: F821
Annotated["PowerOutletType", strawberry.lazy('dcim.graphql.types')], # noqa: F821
Annotated["PowerPortType", strawberry.lazy('dcim.graphql.types')], # noqa: F821
Annotated["ProviderNetworkType", strawberry.lazy('circuits.graphql.types')], # noqa: F821
Annotated["RearPortType", strawberry.lazy('dcim.graphql.types')], # noqa: F821
], strawberry.union("ConnectedEndpointType")]]
2 changes: 1 addition & 1 deletion netbox/dcim/management/commands/trace_paths.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ def handle(self, *model_names, **options):
self.stdout.write((self.style.SUCCESS(f' Deleted {deleted_count} paths')))

# Reinitialize the model's PK sequence
self.stdout.write(f'Resetting database sequence for CablePath model')
self.stdout.write('Resetting database sequence for CablePath model')
sequence_sql = connection.ops.sequence_reset_sql(no_style(), [CablePath])
with connection.cursor() as cursor:
for sql in sequence_sql:
Expand Down
1 change: 0 additions & 1 deletion netbox/dcim/models/device_component_templates.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,6 @@ def clean(self):

def _get_module_tree(self, module):
modules = []
all_module_bays = module.device.modulebays.all().select_related('module')
while module:
modules.append(module)
if module.module_bay:
Expand Down
3 changes: 1 addition & 2 deletions netbox/dcim/tables/devicetypes.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
from django.utils.translation import gettext_lazy as _
import django_tables2 as tables
from django.utils.translation import gettext as _
from django.utils.translation import gettext_lazy as _

from dcim import models
from netbox.tables import NetBoxTable, columns
Expand Down
4 changes: 2 additions & 2 deletions netbox/dcim/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -2135,12 +2135,12 @@ def setUpTestData(cls):
def test_get_connected_device(self):
url = reverse('dcim-api:connected-device-list')

url_params = f'?peer_device=TestDevice1&peer_interface=eth0'
url_params = '?peer_device=TestDevice1&peer_interface=eth0'
response = self.client.get(url + url_params, **self.header)
self.assertHttpStatus(response, status.HTTP_200_OK)
self.assertEqual(response.data['name'], 'TestDevice2')

url_params = f'?peer_device=TestDevice1&peer_interface=eth1'
url_params = '?peer_device=TestDevice1&peer_interface=eth1'
response = self.client.get(url + url_params, **self.header)
self.assertHttpStatus(response, status.HTTP_404_NOT_FOUND)

Expand Down
7 changes: 0 additions & 7 deletions netbox/dcim/tests/test_filtersets.py
Original file line number Diff line number Diff line change
Expand Up @@ -4860,13 +4860,6 @@ def test_device_role(self):
params = {'device_role': [role[0].slug, role[1].slug]}
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 4)

def test_role(self):
role = DeviceRole.objects.all()[:2]
params = {'role_id': [role[0].pk, role[1].pk]}
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 4)
params = {'role': [role[0].slug, role[1].slug]}
self.assertEqual(self.filterset(params, self.queryset).qs.count(), 4)

def test_device(self):
devices = Device.objects.all()[:2]
params = {'device_id': [devices[0].pk, devices[1].pk]}
Expand Down
Loading

0 comments on commit ee15a57

Please sign in to comment.