Add more ruff rules, remove flake8 and black #5039

glennmatthews · 2024-01-02T22:02:47Z

Closes: n/a

What's Changed

Updated ruff development dependency to ~0.1.10 (latest as of today)
Add ruff format to invoke ruff task (before we were only doing ruff check; ruff format replaces black)
Removed black and flake8 as development dependencies as they're fully replaced by ruff now.
Removed black and flake8 steps from CI.
Enabled DJ Ruff rules (flake8-django) and addressed all warnings raised.
- Some of these resulted in me adding TODO comments as the correct resolution wasn't immediately obvious to me
Enabled PIE Ruff rules except for PIE808 (flake8-pie) and addressed all warnings raised.
- PIE808 warns about doing range(0, n) as it's technically redundant with simply range(n), but personally I prefer the clarity of the former style.
Enabled RUF Ruff native rules except for RUF012 (which results in a lot of noise, see also RUF012 triggers many false positives (are they really? they are correct) in some projects astral-sh/ruff#5243) and addressed all warnings raised.
Enabled remaining S Ruff rules (flake8-bandit) and addressed all warnings raised.

If this gets accepted and merged, I can open a sequel PR to remove isort (which is included as a dev dependency, but not actually run by our CI presently) and enable its equivalent rules in ruff; I've already done the work locally but it results in a lot more diffs and I wanted to keep this PR from being too unmanageable to review. 😀

TODO

Explanation of Change(s)
Added change log fragment(s) (for more information see the documentation)
n/a Attached Screenshots, Payload Example
Unit, Integration Tests
Documentation Updates (when adding/changing features)
Example Plugin Updates (when adding/changing features)
Outline Remaining Work, Constraints from Design

glennmatthews · 2024-01-02T22:06:33Z

nautobot/core/choices.py

-            ALPHA = "Α"
-            BETA = "Β"
+            ALPHA = "A"
+            BETA = "B"


Replacing sneaky Unicode Greek letters with their ASCII equivalents; could also have done a # noqa: RUF01 if we wanted to keep them as-is.

glennmatthews · 2024-01-02T22:07:53Z

nautobot/core/models/__init__.py

flake8-django rules didn't like the order we were declaring Meta and get_absolute_url() in BaseModel - surprisingly most of our other models got this "right" already.

glennmatthews · 2024-01-02T22:08:58Z

nautobot/core/runner/importer.py

Removing some sketchy code that really isn't even necessary - it was attempting to "pre-validate" the settings file before actually importing it, but if the settings file is invalid, we still get a perfectly clear error without the removed code.

nautobot/core/testing/filters.py

glennmatthews · 2024-01-02T22:11:58Z

nautobot/core/tests/test_cli.py

Very old code that was not properly implemented for discovery by unittest; I made a few attempts at fixing it to actually run but couldn't get it working properly.

nautobot/core/tests/test_views.py

nautobot/extras/choices.py

glennmatthews · 2024-01-02T22:15:17Z

nautobot/extras/models/jobs.py

I'd appreciate eyes on the TODOs in this file.

I agree with the TODOs. We shouldn't make these changes now but since django_celery_results is using the JobResult model we should make sure that changing these fields doesn't break that module.

nautobot/extras/tests/integration/test_plugins.py

tasks.py

nautobot/ipam/tests/test_filters.py

gsnider2195 · 2024-01-03T16:26:16Z

nautobot/ipam/querysets.py

@@ -305,7 +305,7 @@ def delete(self, *args, **kwargs):
            return super().delete(*args, **kwargs)
        except ProtectedError as err:
            # This will be either IPAddress or Prefix.
-            protected_instance = tuple(err.protected_objects)[0]


While the justification for this rule makes sense I don't like how difficult it is to read this line. Maybe eventually my brain will learn to associate next(iter(list)) with grabbing the first element of list..

Agreed - this is one I considered disabling when I ran across it, because the replacement syntax is definitely non-obvious.

gsnider2195 · 2024-01-03T16:51:38Z

Can you add the ruff extension to the nautobot.code-workspace extensions.recommendations list? It's "charliermarsh.ruff"

nautobot/extras/tests/test_views.py

gsnider2195 · 2024-01-03T19:18:24Z

nautobot/extras/signals.py

@@ -127,7 +127,7 @@ def _handle_changed_object(sender, instance, raw=False, **kwargs):

    # Housekeeping: 0.1% chance of clearing out expired ObjectChanges
    changelog_retention = get_settings_or_config("CHANGELOG_RETENTION")
-    if changelog_retention and random.randint(1, 1000) == 1:
+    if changelog_retention and secrets.randbelow(1000) == 0:


Looks like the performance impact is minimal (default for timeit is 10k iterations)

>>> timeit.timeit(setup="import random", stmt="random.randint(1, 1000)") 1.0842244539999228 >>> timeit.timeit(setup="import secrets", stmt="secrets.randbelow(1000)") 2.6048511999979382 >>> timeit.timeit(setup="import random", stmt="random.randint(1, 1000)") 1.0478729710011976 >>> timeit.timeit(setup="import secrets", stmt="secrets.randbelow(1000)") 1.6280573209987779

Thank you for checking me on this!

Hopefully this particular code is not long for this world in any case... :-)

nautobot/docs/development/core/getting-started.md

Co-authored-by: Gary Snider <75227981+gsnider2195@users.noreply.github.com>

gsnider2195 · 2024-01-03T19:48:37Z

nautobot/core/runner/importer.py

@@ -12,11 +12,6 @@
 from .settings import load_settings, create_module


-def execfile(afile, globalz=None, localz=None):


Can you add some context to the PR for the changes made in this file?

I did in #5039 (comment), looks like file-level comments don't carry over in the GitHub UI when the file changes. :-/

I see now. Do we still get a valid error with an invalid EXTRA_INSTALLED_APPS or EXTRA_MIDDLEWARE without this code?

Invalid would be like the wrong data type (dict instead of list, that sort of thing)? I'd tested earlier with a syntax error in the nautobot_config.py and that seemed fine, but I'm open to doing additional tests.

Wrong data type, or a non-existent python library (for EXTRA_INSTALLED_APPS). Just want to make sure you captured the EXTRA_ settings that this custom importer is providing.

With EXTRA_INSTALLED_APPS = ["hello world"]:

nautobot-nautobot-1 | Traceback (most recent call last): nautobot-nautobot-1 | File "/usr/local/bin/nautobot-server", line 6, in <module> nautobot-nautobot-1 | sys.exit(main()) nautobot-nautobot-1 | File "/source/nautobot/core/cli/__init__.py", line 53, in main nautobot-nautobot-1 | run_app( nautobot-nautobot-1 | File "/source/nautobot/core/runner/runner.py", line 297, in run_app nautobot-nautobot-1 | management.execute_from_command_line([runner_name, command, *command_args]) nautobot-nautobot-1 | File "/usr/local/lib/python3.8/site-packages/django/core/management/__init__.py", line 419, in execute_from_command_line nautobot-nautobot-1 | utility.execute() nautobot-nautobot-1 | File "/usr/local/lib/python3.8/site-packages/django/core/management/__init__.py", line 395, in execute nautobot-nautobot-1 | django.setup() nautobot-nautobot-1 | File "/usr/local/lib/python3.8/site-packages/django/__init__.py", line 24, in setup nautobot-nautobot-1 | apps.populate(settings.INSTALLED_APPS) nautobot-nautobot-1 | File "/usr/local/lib/python3.8/site-packages/django/apps/registry.py", line 91, in populate nautobot-nautobot-1 | app_config = AppConfig.create(entry) nautobot-nautobot-1 | File "/usr/local/lib/python3.8/site-packages/django/apps/config.py", line 224, in create nautobot-nautobot-1 | import_module(entry) nautobot-nautobot-1 | File "/usr/local/lib/python3.8/importlib/__init__.py", line 127, in import_module nautobot-nautobot-1 | return _bootstrap._gcd_import(name[level:], package, level) nautobot-nautobot-1 | File "<frozen importlib._bootstrap>", line 1014, in _gcd_import nautobot-nautobot-1 | File "<frozen importlib._bootstrap>", line 991, in _find_and_load nautobot-nautobot-1 | File "<frozen importlib._bootstrap>", line 973, in _find_and_load_unlocked nautobot-nautobot-1 | ModuleNotFoundError: No module named 'hello world'

With EXTRA_INSTALLED_APPS = {"hello": "world"}:

nautobot-nautobot-1 | Traceback (most recent call last): nautobot-nautobot-1 | File "/usr/local/bin/nautobot-server", line 6, in <module> nautobot-nautobot-1 | sys.exit(main()) nautobot-nautobot-1 | File "/source/nautobot/core/cli/__init__.py", line 53, in main nautobot-nautobot-1 | run_app( nautobot-nautobot-1 | File "/source/nautobot/core/runner/runner.py", line 297, in run_app nautobot-nautobot-1 | management.execute_from_command_line([runner_name, command, *command_args]) nautobot-nautobot-1 | File "/usr/local/lib/python3.8/site-packages/django/core/management/__init__.py", line 419, in execute_from_command_line nautobot-nautobot-1 | utility.execute() nautobot-nautobot-1 | File "/usr/local/lib/python3.8/site-packages/django/core/management/__init__.py", line 395, in execute nautobot-nautobot-1 | django.setup() nautobot-nautobot-1 | File "/usr/local/lib/python3.8/site-packages/django/__init__.py", line 24, in setup nautobot-nautobot-1 | apps.populate(settings.INSTALLED_APPS) nautobot-nautobot-1 | File "/usr/local/lib/python3.8/site-packages/django/apps/registry.py", line 91, in populate nautobot-nautobot-1 | app_config = AppConfig.create(entry) nautobot-nautobot-1 | File "/usr/local/lib/python3.8/site-packages/django/apps/config.py", line 224, in create nautobot-nautobot-1 | import_module(entry) nautobot-nautobot-1 | File "/usr/local/lib/python3.8/importlib/__init__.py", line 127, in import_module nautobot-nautobot-1 | return _bootstrap._gcd_import(name[level:], package, level) nautobot-nautobot-1 | File "<frozen importlib._bootstrap>", line 1014, in _gcd_import nautobot-nautobot-1 | File "<frozen importlib._bootstrap>", line 991, in _find_and_load nautobot-nautobot-1 | File "<frozen importlib._bootstrap>", line 973, in _find_and_load_unlocked nautobot-nautobot-1 | ModuleNotFoundError: No module named 'hello'

Looks like the same stack trace I get in develop in both cases -- which makes sense as I think about it, since all the removed code was doing was checking whether the config file could be imported, not whether it's actually semantically valid.

With a syntax error in the config file, develop gives:

nautobot-nautobot-1 | Traceback (most recent call last): nautobot-nautobot-1 | File "/source/nautobot/core/runner/importer.py", line 72, in validate nautobot-nautobot-1 | execfile(self.config_path, {"__file__": self.config_path}) nautobot-nautobot-1 | File "/source/nautobot/core/runner/importer.py", line 17, in execfile nautobot-nautobot-1 | exec(fh.read(), globalz, localz) nautobot-nautobot-1 | File "<string>", line 74 nautobot-nautobot-1 | EXTRA_INSTALLED_APPS = { nautobot-nautobot-1 | ^ nautobot-nautobot-1 | SyntaxError: unexpected EOF while parsing nautobot-nautobot-1 | nautobot-nautobot-1 | During handling of the above exception, another exception occurred: nautobot-nautobot-1 | nautobot-nautobot-1 | Traceback (most recent call last): nautobot-nautobot-1 | File "/usr/local/bin/nautobot-server", line 6, in <module> nautobot-nautobot-1 | sys.exit(main()) nautobot-nautobot-1 | File "/source/nautobot/core/cli/__init__.py", line 53, in main nautobot-nautobot-1 | run_app( nautobot-nautobot-1 | File "/source/nautobot/core/runner/runner.py", line 292, in run_app nautobot-nautobot-1 | configure_app(config_path=config_path, **kwargs) nautobot-nautobot-1 | File "/source/nautobot/core/runner/runner.py", line 131, in configure_app nautobot-nautobot-1 | importer.install( nautobot-nautobot-1 | File "/source/nautobot/core/runner/importer.py", line 37, in install nautobot-nautobot-1 | sys.meta_path.insert(0, LoganImporter(name, config_path, default_settings, **kwargs)) nautobot-nautobot-1 | File "/source/nautobot/core/runner/importer.py", line 63, in __init__ nautobot-nautobot-1 | self.validate() nautobot-nautobot-1 | File "/source/nautobot/core/runner/importer.py", line 75, in validate nautobot-nautobot-1 | raise ConfigurationError(str(e), exc_info[2]) nautobot-nautobot-1 | nautobot.core.runner.importer.ConfigurationError: ('unexpected EOF while parsing (<string>, line 74)', <traceback object at 0x7fd1a1519340>)

while this branch instead gives:

nautobot-nautobot-1 | Traceback (most recent call last): nautobot-nautobot-1 | File "/usr/local/bin/nautobot-server", line 6, in <module> nautobot-nautobot-1 | sys.exit(main()) nautobot-nautobot-1 | File "/source/nautobot/core/cli/__init__.py", line 53, in main nautobot-nautobot-1 | run_app( nautobot-nautobot-1 | File "/source/nautobot/core/runner/runner.py", line 292, in run_app nautobot-nautobot-1 | configure_app(config_path=config_path, **kwargs) nautobot-nautobot-1 | File "/source/nautobot/core/runner/runner.py", line 145, in configure_app nautobot-nautobot-1 | hasattr(settings, "INSTALLED_APPS") nautobot-nautobot-1 | File "/usr/local/lib/python3.8/site-packages/django/conf/__init__.py", line 82, in __getattr__ nautobot-nautobot-1 | self._setup(name) nautobot-nautobot-1 | File "/usr/local/lib/python3.8/site-packages/django/conf/__init__.py", line 69, in _setup nautobot-nautobot-1 | self._wrapped = Settings(settings_module) nautobot-nautobot-1 | File "/usr/local/lib/python3.8/site-packages/django/conf/__init__.py", line 170, in __init__ nautobot-nautobot-1 | mod = importlib.import_module(self.SETTINGS_MODULE) nautobot-nautobot-1 | File "/usr/local/lib/python3.8/importlib/__init__.py", line 127, in import_module nautobot-nautobot-1 | return _bootstrap._gcd_import(name[level:], package, level) nautobot-nautobot-1 | File "<frozen importlib._bootstrap>", line 1014, in _gcd_import nautobot-nautobot-1 | File "<frozen importlib._bootstrap>", line 991, in _find_and_load nautobot-nautobot-1 | File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked nautobot-nautobot-1 | File "<frozen importlib._bootstrap>", line 655, in _load_unlocked nautobot-nautobot-1 | File "<frozen importlib._bootstrap>", line 618, in _load_backward_compatible nautobot-nautobot-1 | File "/source/nautobot/core/runner/importer.py", line 115, in load_module nautobot-nautobot-1 | load_settings(self.config_path, allow_extras=self.allow_extras, settings=settings_mod) nautobot-nautobot-1 | File "/source/nautobot/core/runner/settings.py", line 47, in load_settings nautobot-nautobot-1 | spec.loader.exec_module(conf) nautobot-nautobot-1 | File "<frozen importlib._bootstrap_external>", line 839, in exec_module nautobot-nautobot-1 | File "<frozen importlib._bootstrap_external>", line 976, in get_code nautobot-nautobot-1 | File "<frozen importlib._bootstrap_external>", line 906, in source_to_code nautobot-nautobot-1 | File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed nautobot-nautobot-1 | File "/opt/nautobot/nautobot_config.py", line 78 nautobot-nautobot-1 | EXTRA_INSTALLED_APPS = { nautobot-nautobot-1 | ^ nautobot-nautobot-1 | SyntaxError: unexpected EOF while parsing

which I actually think is an improvement?

Agreed that it's an improvement. I'm happy with it, thanks for testing that out.

gsnider2195

I'd like some more background on the changes made in nautobot/core/runner/importer.py but otherwise I think this is a big performance improvement and I don't see any loss of functionality from flake8/black. Great stuff!

glennmatthews added 8 commits January 2, 2024 13:55

Enable DJ (flake8-django) ruff rules

26c0f34

Enable and address remaining ruff 'S' checks

7a176e6

Enable RUF ruff rules

6a29d4d

Enable PIE ruff rules except PIE808

c7a1608

Update to ruff 0.1.10, remove black and flake8

9eb0c59

Placeholder change fragment

8354142

Renumber change fragment

2c51bba

Merge branch 'develop' into u/glennmatthews-more-ruff

21f1ff8

glennmatthews commented Jan 2, 2024

View reviewed changes

glennmatthews added 3 commits January 3, 2024 09:20

Fix tempdir reference in test

c0587a1

Replace some 'random' usage with 'secrets' equivalents

04b95d7

Docs update

580be73