use ruff for linting

[c0rydoras]

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)

[ikelos]

Can you explain the pros (and cons) of using ruff over black please?

[c0rydoras]

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 (ruff) for linting and formatting instead of 2 (ruff for linting and black for formatting)

[ikelos]

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]

ruff is now only used for linting

[ikelos]

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?

[c0rydoras]

Deciding on the ignore list will be tricky...

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/

[ikelos]

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...

[c0rydoras]

ruff can fix the lambda one by itself, I just thought it looked uglier in the diff

[ikelos]

The CodeQL error we can fix up by moving the skip check up inside the exception handler just above it (ie, if there's no basename, it should still keep going, which seemed to be the intention of the original code.

Thank you so much for putting all the effort in to get this where it is! I realize there's been a lot of nitpicking, but I really appreciate you having taken the time to work through and fix them all, it's much appreciated!

[c0rydoras]

what does this check do?

            if self.config["name"] and self.config["name"] not in BaseDllName:
                continue

AFAICT this just always continues if self.config["name"]

[ikelos]

Because of the and, it only skips it if there's a name in the config that isn't the name mentioned in BaseDllName, so it's a filtration to only select unnamed things or the one that's specifically named.

[ikelos]

If there's no name, everything gets output, if there's no BaseDLLName, it should get included (I believe) and then only when both the filter is set and the filter condition is met, are the items filtered.

[ikelos]

Awesome, looking good, tests are passing, just need some of the guys to comment on whether they're happy with us linting with it...