use ruff for linting #1364
use ruff for linting #1364
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -794,7 +789,7 @@ | |||
| fd, self._name = tempfile.mkstemp( | |||
| suffix=".vol3", prefix="tmp_", dir=output_dir | |||
| ) | |||
| self._file = io.open(fd, mode="w+b") | |||
| self._file = open(fd, mode="w+b") | |||
Check warning
Code scanning / CodeQL
File is not always closed Warning
c0rydoras
force-pushed
the
linting
branch
6 times, most recently
from
e89eacd to
b789f53
Compare
|
Can you explain the pros (and cons) of using ruff over black please? |
|
ruff is more than black, it has a formatter but it started as a linter, it (the formatter) is faster than black and more configurable, it can also format docstrings its linter is not only faster than other linting tools (e.g. flake8, pylint etc.) but it also supports a lot of those linters (e.g. https://docs.astral.sh/ruff/rules/#flake8-builtins-a, https://docs.astral.sh/ruff/rules/#pylint-pl) (and a lot more rules in general) the only real change here is that there's faster formatting (it might deviate a bit from black, see https://docs.astral.sh/ruff/formatter/black/) and just one dependency ( |
|
Thanks, I'm a little suspicious that there weren't any downsides to using ruff, that's why I asked for pros and cons. I've run some tests and there's 9 files that black and ruff can't agree on how to format. I also kinda liked black in that it wasn't configurable, and so you wouldn't get bickering about people wanting the formatting done a particular way. Would it be possible to add in ruff just as a linter initially, get those issues fixed up if possible (I prefer to fix rather than ignore, because it's either a bad rule that can be ignored anywhere, or it's a good rule that should always be followed) and then consider shifting the formatter over to ruff from black once the dust has cleared? |
c0rydoras
force-pushed
the
linting
branch
4 times, most recently
from
8c8e05a to
d9b786b
Compare
|
ruff is now only used for linting |
|
Thanks very much, I'm generally happy with the changes it's made. There's a couple places where it's decided not including defaults is better than being explicit, which I can get used to. Would it be possible to make the suggested changes so get rid of the errors that ruff is reporting in the code base, or would that need to be done in a second commit (just worried this'll be importing a new test that then immediately fails, and I'd prefer we knew it wouldn't impede new commits). Deciding on the ignore list will be tricky... I think the module imports can move up, and I understand the reasoning behind not using f-strings in argparse, so I think we could manually make the necessary change without breaking anything. Would you be up for making those please? |
right now its configured to only use a few rules, there might be other ones we want to enable: https://docs.astral.sh/ruff/rules/ |
|
Yeah, I'd prefer as few exceptions as possible. I'd even be ok with the lambda assignment one coming back in (assuming it wasn't difficult to fix the places it was complaining about)? Just trying to ensure we don't get time wasted on tweak or fixing than we do on agreeing and writing code that adheres to our standard... |
|
ruff can fix the lambda one by itself, I just thought it looked uglier in the diff |
c0rydoras
force-pushed
the
linting
branch
3 times, most recently
from
05cc386 to
383fdc9
Compare
the import is not needed, was previously used for type annotations
When volatilityfoundation#1364 was merged, it may not have been rebased onto the changes introduced in volatilityfoundation#1422, and they ended up overwritten to the old version. This reverts those changes.
When volatilityfoundation#1364 was merged, it may not have been rebased onto the changes introduced in volatilityfoundation#1422, and they ended up overwritten to the old version. This reverts those changes.
|
@c0rydoras Any idea why this didn't fail the ruff test we put in place? Just want to make sure it's operating as expected... |
right now the CI should fail because some rules can't safely be fixed automatically
before I start fixing/ignoring things to make the CI pass, I'd like to know if linting like this is even something you (the maintainers) would want
(if this were ever to land it would make sense to add the commits to a
.git-blame-ignore-revs; see https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view)