Evaluate using HomeAssistant rules in ruff

[alexrudd2]

If it makes things simpler then please make a PR that changes from bandit to flake8-bandit.

Can you please ( if you have not already) the rules from HA, commented out, that way it is easier to activate when you find t8mr.

Originally posted by @janiversen in #1476 (comment)

https://github.com/home-assistant/core/blob/7eccef87c2cccb834f5ff394f765e697831390e3/pyproject.toml#L240

• "B007", # Loop control variable {name} not used within loop body
• "B014", # Exception handler with duplicate exception
• "C", # complexity
• "D", # docstrings
• "E", # pycodestyle
• "F", # pyflakes/autoflake
• "ICN001", # import concentions; {name} should be imported as {asname}
• "PGH004", # Use specific rule codes when using noqa
• "PLC0414", # Useless import alias. Import alias does not rename original package.
• "SIM105", # Use contextlib.suppress({exception}) instead of try-except-pass
• "SIM117", # Merge with-statements that use the same scope
• "SIM118", # Use {key} in {dict} instead of {key} in {dict}.keys()
• "SIM201", # Use {left} != {right} instead of not {left} == {right}
• "SIM212", # Use {a} if {a} else {b} instead of {b} if not {a} else {a}
• "SIM300", # Yoda conditions. Use 'age == 42' instead of '42 == age'.
• "SIM401", # Use get from dict with default instead of an if block
• "T20", # flake8-print
• "TRY004", # Prefer TypeError exception for invalid type
• "RUF006", # Store a reference to the return value of asyncio.create_task
• "UP", # pyupgrade
• "W", # pycodestyle

ignore = [

• "D202", # No blank lines allowed after function docstring
• "D203", # 1 blank line required before class docstring
• "D213", # Multi-line docstring summary should start at the second line
• "D406", # Section name should end with a newline
• "D407", # Section name underlining
• "E501", # line too long
• "E731", # do not assign a lambda expression, use a def
  Ignored due to performance: Implement PEP 604 rewrites for isinstance and issubclass checks astral-sh/ruff#2923
• "UP038", # Use X | Y in isinstance call instead of (X, Y)

[janiversen]

I did not express myself well, please add these rules in our ruff.toml but comment those out that currently gives you a headache.

This PR is clean and nice, let's keep it like that, and then you can un comment the rules in new PR(s).

[alexrudd2]

I did not express myself well, please add these rules in our ruff.toml but comment those out that currently gives you a headache.

I understood you, but remember better if it's an issue assigned to me. (I work across a dozen projects)

[janiversen]

Ahh it is a new one, sorry I am on my iPAD this evening.

[janiversen]

Looking forward to see more PR around this theme.

I too work on a number of projects and I have absolutely no problem with you opening issues as reminders.

[alexrudd2]

See #1480 and #1481

[janiversen]

Looks like you have the version active ? or am I missing something?

[alexrudd2]

Looks like you have the version active ? or am I missing something?

I don't understand your question.

===

Anyways, I've gone and deleted some rules where I don't see value (such as flake8-print), and enabled a few more easy ones.

There are three remaining "big" rulesets which I think are useful, but require more substantial work. I've put them in but commented out.

• bandit
• Tryceratops
• McCabe complexity

[janiversen]

we already use bandit directly and mccabe via pylint, so it puzzles me why those 2 are big jobs....the third one is new to me.

Anyhow if you want we can divide them, or you do it slowly when you have time.

[alexrudd2]

we already use bandit directly and mccabe via pylint, so it puzzles me why those 2 are big jobs....the third one is new to me.

For McCabe/pylint, I intend to actually reduce the complexity and not just use pylint: ignore :)

For bandit, I don't think it's actually used:

pymodbus/check_ci.sh

Lines 8 to 13 in 248f8de

	codespell
	pre-commit run --all-files
	pylint --recursive=y examples pymodbus test
	mypy pymodbus
	pytest --numprocesses auto
	echo "Ready to push"

Both ruff with the bandit rules and bandit with the default config report violations currently.

Code scanned:
	Total lines of code: 21974
	Total lines skipped (#nosec): 0

Run metrics:
	Total issues (by severity):
		Undefined: 0
		Low: 893
		Medium: 1
		High: 0
	Total issues (by confidence):
		Undefined: 0
		Low: 0
		Medium: 1
		High: 893
Files skipped (0):

tryceratops is now to me also, but it might be useful for improved exception handling.

Anyhow if you want we can divide them, or you do it slowly when you have time.

[janiversen]

Bandit is used:

pymodbus/.github/workflows/ci.yml

Line 34 in 248f8de

- name: bandit

Reducing is a nice goal, but please please do it very carefully, and in chunks I can review....I have already tried and failed (or at least postponed).

[alexrudd2]

Bandit is used:

o_0. OK, I missed that. Hopefully #1488 makes it clearer (and kills another CI runner, yay)

However, ruff still doesn't actually have full parity with flake8-bandit so I think it's best to wait a bit. (astral-sh/ruff#1646)

Reducing [the McCabe complexity] is a nice goal, but please please do it very carefully, and in chunks I can review....I have already tried and failed (or at least postponed).

Agreed, it's difficult!

[janiversen]

I believe you do !

[janiversen]

I just read about ruff, in another project. Is rule M001 default ? if not we should activate it, to get warnings when there are unused noga statements.

[alexrudd2]

Hmm, it appears M001 has been remapped to RUF100. This would be part of the RUF rules, which are not enabled by default, BUT are enabled for pymodbus here.

I'd suggest moving the other project from M001 to RUF100 if they haven't already.

[janiversen]

thanks.

[alexrudd2]

we already use bandit directly and mccabe via pylint, so it puzzles me why those 2 are big jobs....the third one is new to me.

For McCabe/pylint, I intend to actually reduce the complexity and not just use pylint: ignore :)

If #1510 is OK, we're down to two functions with a McCabe complexity of > 10.

pymodbus/transaction.py:127:9: C901 execute is too complex (23 > 10)
pymodbus/transaction.py:335:9: C901 _recv is too complex (19 > 10)

Note that both of them have large if/else if/else blocks that are functionally switch statements. If
astral-sh/ruff#3597 gets implemented that will reduce their scores.

[janiversen]

I have reworked many if/else constructs to
if - return...and no else. I try in general not to have nested if/else.

So I suggest you follow the same pattern if possible.

[alexrudd2]

I have reworked many if/else constructs to if - return...and no else. I try in general not to have nested if/else.

So I suggest you follow the same pattern if possible.

Yes, it's good advice when possible for nested if/else. However, I meant code like this (sequential if / else if / else) that would be a switch/case or match in other languages:

pymodbus/pymodbus/transaction.py

Lines 340 to 349 in 759b222

	if isinstance(self.client.framer, ModbusSocketFramer):
	min_size = 8
	elif isinstance(self.client.framer, ModbusRtuFramer):
	min_size = 4
	elif isinstance(self.client.framer, ModbusAsciiFramer):
	min_size = 5
	elif isinstance(self.client.framer, ModbusBinaryFramer):
	min_size = 3
	else:
	min_size = expected_response_length

Actually, since there are now only a few left I think I will just ignore them, switch to ruff, and disable the pylint McCabe.

[janiversen]

Switch to ruff sounds like a good idea, but please do not disable McCabe, but add "pylint: disable=complex..." or whatever it is called in ruff, for those couple of methods.

[alexrudd2]

Switch to ruff sounds like a good idea, but please do not disable McCabe, but add "pylint: disable=complex..." or whatever it is called in ruff, for those couple of methods.

I meant disabling the pylint McCade while enabling it in ruff. a25de7f

[janiversen]

Ahh got it, my bad 😀