feat: introduce the Ruff pre-commit hook to replace a bunch of other code formatters and linter hooks

[jenstroeger]

Summary

This change replaces black, bandit, isort, flake8, and pyupgrade git hooks with a single ruff hook.

Description of changes

Ruff is supposed to be a drop-in replacement for the aforementioned checkers, linters, and code formatters and it mostly works. However, it also

• reformatted a few lines of code;
• complained about a few issues here and there;
• complained about FooBarException which should be named FooBarError which could be considered a breaking change;
• required renaming bandit comments nosec Bxxx to noqa: Sxxx.

In addition to the rules covering the existing Macaron setup, Ruff provides a bunch of new rules and checkers (e.g. boolean trap) that are probably interesting to add… 🤓

Related issues

n/a

Checklist

• I have reviewed the contribution guide.
• My PR title and commits follow the Conventional Commits convention.
• My commits include the "Signed-off-by" line.
• I have signed my commits following the instructions provided by GitHub. Note that we run GitHub's commit verification tool to check the commit signatures. A green verified label should appear next to all of your commits on GitHub.
• I have updated the relevant documentation, if applicable.
• I have tested my changes and verified they work as expected.

[jenstroeger]

@behnazh-w in addition to the existing checks, perhaps it would make sense to add the following:

• FBT (check for boolean trap)
• DTZ (ban the usage of unsafe naive datetime class)
• ICN (how certain packages should be imported or aliased)
• PIE (assortment of various checks, partially overlaps with others like Bugbear or pylint)
• SIM (various tips about simplifying code)
• SLOT (ensure __slots__ for subclasses of immutables like str)
• TID (additional tidying up of imports)
• PERF (see also perflint, a plugin we removed due to inactivity)
• DOC and D (check and enforce docstring comments)
• F (check various lint and flakes)
• PL (this is pylint but also consider Implement Pylint astral-sh/ruff#970)
• FURB (similar to pyupgrade)
• RUF (these are additional checks implemented by Ruff)

Also, note that there’s a CPY (copyright) rule which might replace this:

macaron/.pre-commit-config.yaml

Lines 203 to 211 in 109edd2

	# Checks the copyright header for .js, .py, and .java, etc. files.
	- repo: local
	hooks:
	- id: copyright-checker
	name: Copyright checker
	entry: scripts/dev_scripts/copyright-checker.sh
	language: system
	always_run: true
	pass_filenames: false

[jenstroeger]

@behnazh-w I am a little confused about this comment. For one, it’s a blanket ignore and it doesn’t specify what error is being ignored here; this line should be

status_on_skipped,  # type: ignore[arg-type]

But then the question arises: why is this argument type being ignored? Looking at the signature for the initializer

macaron/src/macaron/slsa_analyzer/checks/base_check.py

Lines 24 to 34 in 109edd2

	class BaseCheck:
	"""This abstract class is used to implement Checks in Macaron."""
	def __init__(
	self,
	check_id: str = "",
	description: str = "",
	depends_on: list[tuple[str, CheckResultType]] | None = None,
	eval_reqs: list[ReqName] | None = None,
	result_on_skip: CheckResultType = CheckResultType.SKIPPED,
	) -> None:

the type for the status_on_skipped variable should be

macaron/src/macaron/slsa_analyzer/checks/check_result.py

Lines 15 to 26 in 109edd2

	class CheckResultType(str, Enum):
	"""This class contains the types of a check result."""
	PASSED = "PASSED"
	FAILED = "FAILED"
	# A check is skipped from another check's result.
	SKIPPED = "SKIPPED"
	# A check is disabled from the user configuration.
	DISABLED = "DISABLED"
	# The result of the check is unknown or Macaron cannot resolve the
	# implementation of this check.
	UNKNOWN = "UNKNOWN"

However, that status_on_skipped variable is the test function‘s argument

macaron/tests/slsa_analyzer/checks/test_registry.py

Lines 156 to 162 in 109edd2

	@given(
	lists(
	one_of(none(), text(), integers(), tuples(), binary(), booleans()),
	min_size=1,
	)
	)
	def test_exit_on_invalid_status_on_skipped(self, status_on_skipped: SearchStrategy) -> None:

declared as a Hypothesis SearchStrategy. But that’s not quite right because @given feeds values from strategies into the test function and not the strategy object. Thus, the function argument receives a list[None | str | int | tuple | bytes | bool] but that still doesn’t match the signature’s CheckResultType. So shouldn’t this test look something like

@given(st.sampled_from(list(CheckResultType)))
def test_exit_on_invalid_status_on_skipped(self, status_on_skipped: CheckResultType) -> None:
    ...

I’ve not tested this.

---

I then grepped through the code base and there are heaps of blanket type ignores. I think it would therefore make sense to open an issue to review all these blanket ignores, attempt to address them by fixing up the code (see above), and if that’s not possible constrain the ignore to only the error that’s being ignored. By doing so, other type errors will be surfaced if they occur instead of silently swallowing all errors.