project: Extend ruff check with pycodestyle, flake8-bugbear, pyflakes #764
Conversation
src/west/app/main.py
Outdated
| f'ignoring project {spec.project.name} ' | ||
| f'extension command "{spec.name}"; ' | ||
| 'this is a built in command')) | ||
| continue | ||
| if spec.name in extension_names: | ||
| self.queued_io.append( | ||
| lambda cmd: cmd.wrn( | ||
| lambda cmd, spec=spec: cmd.wrn( |
There was a problem hiding this comment.
Wait, this one is not just silencing a warning: this looks like an actual bug! So it needs a different commit message.
Unsurprisingly, no one ever complained because this is error handling code which is rarely used and never tested.
So I think the bug should go like this: when west is "ignoring" a project, the error message will always refer to the last specs[len(spec)-1] no matter which spec was actually involved.
How would one reproduce this wrong error message? I mean interactively at least. If that's complicated then feel free to drop this commit from this cleanup PR so it can be merged quickly.
There was a problem hiding this comment.
I trust the spec=spec fix suggested by https://docs.astral.sh/ruff/rules/function-uses-loop-variable/ is functionally correct, however it's pretty horrible style because it completely obscures the very subtle issue. The two should be different, something like this:
for spec in specs:
if spec.name in self.builtins:
self.queued_io.append(
lambda cmd, spec_const=spec: cmd.wrn(
f'ignoring project {spec_const.project.name} '
f'extension command "{spec_const.name}"; '
'this is a built in command'))There was a problem hiding this comment.
The issue won't occur once the check has been enabled, I wanted to make as little changes as possible to also preserve git history.
But I can make the changes.
Enable pycodestyle rules when running ruff check. Signed-off-by: Pieter De Gendt <[email protected]>
Derived from flake8-bugbear, see https://docs.astral.sh/ruff/rules/assert-false/ Signed-off-by: Pieter De Gendt <[email protected]>
|
Updated the commit that fixed the actual bug with a more descriptive message. |
There was a problem hiding this comment.
I'm still disappointed by the commit message of the bug fix, sorry.
I'm aware no one ever cares about error handling and error messages (at least not until it hurts them personally) but I think someone experiencing this bug should be able to look at the git log of a newer west version and at least think "Mmmm... could that commit be a fix for this problem I observe?" I don't think that would happen right now.
Did you try to reproduce this bug? If that takes too long, set aside that commit for now so we can merge all the rest?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=a2fe16b9d878a101b6767
"A patch subject line should describe the change, not the tool that found it"
I'll try to describe it better or even add a testcase.
Setting the commit aside, would mean we can't enable |
The 'spec' loop variable was not captured, and this would result in the lambda function only evaluating to the last value in the loop. The issue could occur when loading extension commands from projects, where the name conflicts with a builtin command or an already loaded extension command. Discovered with flake8-bugbear, see https://docs.astral.sh/ruff/rules/function-uses-loop-variable/ Fix function-uses-loop-variable (B023) Signed-off-by: Pieter De Gendt <[email protected]>
Derived from flake8-bugbear, see https://docs.astral.sh/ruff/rules/unused-loop-control-variable/ Signed-off-by: Pieter De Gendt <[email protected]>
Derived from flake8-bugbear, see https://docs.astral.sh/ruff/rules/raise-without-from-inside-except/ Signed-off-by: Pieter De Gendt <[email protected]>
Enable flake8-bugbear rules when running ruff check. Signed-off-by: Pieter De Gendt <[email protected]>
Enable Pyflakes rules when running ruff check. Signed-off-by: Pieter De Gendt <[email protected]>
Thanks! An automated testcase could be overkill, I'm certainly not asking for that.
There's quite a lot more in this PR. There might also be a way to tell |
|
I've updated the commit message again. I'm sorry to put the ball back in your camp, but what would be an acceptable message? I'm not a fluent writer for these things, and I want to do it right, but it should be easy to make progress here. |
Enable pycodestyle, flake8-bugbear, pyflakes rules when running ruff check.