More ruff checks, and make it fix

[boxydog]

Pull Request Checklist

Resolves: #3216

• Ensured tests pass and (if applicable) updated functional test output

Note: there are no manual python fixes in this PR, it's all ruff.

Also, making ruff and ruff-format both fix means if there are pre-commit failures, you can just run it again and it might pass. This is a common pattern for me: run commit once (auto-fixes), run again (commits).

[boxydog]

I added "noqa: I001" and the import line stays in place. Now when I try to look at the pull request to see tests, github is giving me 404s half the time, so I think it's flaky at the moment.

[avaris]

Given the quirks so far, I will have to go through the diff in detail to make sure nothing strange is going on.

[boxydog]

Is this PR ready?

[boxydog]

Given the quirks so far, I will have to go through the diff in detail to make sure nothing strange is going on.

Sorry, I missed this message. Works for me.

[boxydog]

Rebased to current main. Thoughts?

[avaris]

I don't get this. INP has only one rule INP001. What's the point of adding checks for it but ignoring the only thing it checks? I didn't look at all of them but there are other similar ones. e.g. ISC has 3 rules but two off them are ignored or T20 has two rules (but practically T201 is the most common one) and that is ignored...

[boxydog]

I just did the quick thing: added each thing in the list, suppressed those subrules with violations, figuring there were probably other rules being checked. It's like closing the gate, so we don't introduce new violations.

You've found a couple of weird cases, it's true, but I don't understand how you'd like to move forward.

I thought it was probably useful to enforce a bunch of the rules quickly. Perhaps you disagree, but what now?

[avaris]

I just want to understand the logic behind some checks.

• INP for example disallows implicit namespace packages. That's not going to work. We have namespace packages and will probably add more. Why should we disallow that? I understand the reason behind the rule: "Hey, maybe you forgot to add __init__.py?" but that's not applicable here. We intentionally use implicit namespace packages.
• T20 disallows print (or its cousin pprint), which is a very project specific convention. We clearly have scripts that print stuff (pelican-quickstart etc). Either we don't care about this rule or we switch from print to something else. Until then, this rule is effectively not enforceable.
• ISC is one thing I can agree with but we are ignoring most of them :).

I understand that to un-ignore some of these rules, perhaps some manual fixes are needed. That is fine but then I'd treat this as a "work-in-progress" and the ignored rules more like a "to-do list" :).

[boxydog]

I really appreciate your careful review. Thanks.

My overall goal is to make some progress here, merge something. In particular, the auto-fix ("--fix") and auto-format.

I agree the ignored rules are like a TODO list. I was hoping to merge that TODO list, since it represents some knowledge that might be useful to lay out in the codebase.

However, if you don't want to merge that, then another possibility is to take out the extra rules and the suppressions.

What is the quickest path to moving forward?

[boxydog]

Another possibility would be to not include any rules that have suppressions, and drop INP and T20. Not sure where that would leave the list, I'd have to try it.

[avaris]

Also probably drop these comments:

suppression in order of # of violations:
TODO: these only have one violation each:

While they might be accurate at the time, these kind of comments have a very strong tendency to become incorrect and/or misleading quickly.

[boxydog]

Good idea. Done.

[boxydog]

"Done" for adding the comments.

For dropping, how about I add "in Dec 2023"? That would preserve the info (these were in order) without lying.

If I were to work on the TODO list, I'd start with the ones that had one violation each in 2023, it might be helpful to have them marked.

[boxydog]

I added the "in Dec 2023". But I'm happy to drop those comments entirely if you'd prefer.

[justinmayer]

I agree that comments next to rules are helpful. As I've mentioned before, the set of rules and ignores that I've used for some of my plugin code-style check experimentation is an example of that practice: https://github.com/pelican-plugins/featured-image/blob/3d3473269c27dfd3ae123a73aebeb1af4d8b3dd0/pyproject.toml#L58-L91

[justinmayer]

@boxydog: You kindly asked, “What is the quickest path to moving forward?”, and yet the moving forward has not been very quick. Please accept my sincere apologies for that, as well as my gratitude for your patience. I made a few changes to bring the branch current with code that has been merged to master in the interim.

If you are still willing to work on the TODO list as you indicated a few months ago, then I for one would be happy to merge this PR in its current form. Would you be amenable to that?

@avaris: Any follow-up thoughts on the current state of this PR?

(Note: Windows test failures in CI are unrelated — see #3299)

[boxydog]

I would be happy to merge this PR in its current form. Would you be amenable to that?

Sounds good to me.

[boxydog]

If you are still willing to work on the TODO list

Oh, I'd be willing to work on TODOs as well. I have no particular order in mind, tho. Open to suggestions. I'd probably just pick a couple marked as TODO, uncomment them, and see what ruff does.

[boxydog]

Alternatively, I could try to pick some of the TODOs at the bottom of the "ignore" list and see if I can get rid of them. (The ones at the bottom have only one violation apiece.) Again, I'm open to suggestions.

[boxydog]

So where is this PR? I have no planned work. At one point you had said you'd merge it as-is.

I'd suggest adding --exit-non-zero-on-fix to the pre-commit filter (so it fails in the github check if anything had to be fixed). I can do that if you wish. Other than that, I'd merge it.

If you wished, I could work on other ruff changes in a later PR.