Add Ruff to pre-commit (before isort + flake8 to test it as a possible replacement)

mfisher87 commented 1 year ago

Description

I was doing some development for #485 and felt that flake8 was uncomfortably slow to be running in pre-commit (same with Mypy, but one thing at a time ;) ). I've really loved using Ruff on my other projects so I thought I'd propose swapping over with a minimal PR that aims for consistency with the old ruleset and autofixing a few new rules I felt were uncontroversial based on the changes in #420.

netlify[bot] commented 1 year ago

Deploy Preview for conda-lock ready!

Name	Link
Latest commit	52cb31b69a87ca879c5c3e8228e2125c16932bef
Latest deploy log	https://app.netlify.com/sites/conda-lock/deploys/64ee8bedd02a270008d6dbef
Deploy Preview	https://deploy-preview-490--conda-lock.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

mfisher87 commented 1 year ago

PR got autoclosed because of a rebase mistake, woops.

mfisher87 commented 1 year ago

I'm OK with whatever you all want to go with :) I noticed flake8 being slow because I'm working sometimes on an older Thinkpad.

To provide my personal experience with Flake8 vs Ruff (and probably sound like a gushing fan, and I am), I've been using flake8 for about 7 years now as a valued daily tool, but have replaced it with Ruff for every project I start or revisit because I truly feel Ruff has far overtaken it in every way. It being fast is just a (really nice) bonus to me.

I was a big user of flake8 plugins and I was bringing my favorites like bugbear, f-strings, comprehensions, quotes, with my to every project. When I migrated QGreenland from flake8 to Ruff, we were using 7 flake8 plugins, and after the migration, we're checking more things with Ruff alone.
Ruff can autofix almost everything. The amount of time and typing strain this saves me has been life-changing.

The combination of the two above things, more rules + autofixing, means I can do amazing things like change Ruff's target-version and have the U ruleset automatically migrate my type annotations to the newest way, e.g. Typing.tuple[...] -> tuple[...]. All I need to do is review it. I've seen it do some surprisingly complicated changes, and it's got my trust at this point (though I still review its changes) :)

I the best way to try it out is as you described, putting Ruff first but keeping flake8 and isort on board until the team feels they are not adding anything! I'll push a commit to that effect now.

mfisher87 commented 1 year ago

Unfortunately, isort is reversing a change made by Ruff. I really disagree with isort's choice here :) On the subject, the isort author endorsed Ruff as a replacement fairly strongly.

diff --git a/conda_lock/lockfile/v2prelim/models.py b/conda_lock/lockfile/v2prelim/models.py
index 2cc35c2..038a7e1 100644
--- a/conda_lock/lockfile/v2prelim/models.py
+++ b/conda_lock/lockfile/v2prelim/models.py
@@ -7,12 +7,10 @@ from conda_lock.lockfile.v1.models import (
     GitMeta,
     HashModel,
     InputMeta,
-    LockMeta,
-    MetadataOption,
-    TimeMeta,
 )
 from conda_lock.lockfile.v1.models import LockedDependency as LockedDependencyV1
 from conda_lock.lockfile.v1.models import Lockfile as LockfileV1
+from conda_lock.lockfile.v1.models import LockMeta, MetadataOption, TimeMeta
 from conda_lock.models import StrictModel

Having isort and ruff in the pre-commit chain puts the two tools into a disagreement loop. There may be a way to config them to agree more...

mfisher87 commented 1 year ago

I think GitHub is having some problem with the PR sync process, because I pushed a commit to this branch on my fork (GitHub correctly shows that commit was pushed), but the commit isn't showing up in this PR. There's an incident right now, maybe related.

maresb commented 1 year ago

Ya, a few months ago there were several days of GitHub incidents where I was observing similar behavior of disappearing pushes.

I'm seeing 95f64ea. @mfisher87 is that your latest? If so, let's get this merged!

maresb commented 1 year ago

Since when is the Windows test passing? I wonder if that's thanks to the new Mamba/Micromamba releases?

Also, @mfisher87, would you like to prepare a PR with a commit reverting 95f64ea?

mfisher87 commented 1 year ago

Since when is the Windows test passing?

:star_struck: and in less than 30 minutes! That's a big difference from timing out in 6 hours.

I'm seeing 95f64ea. @mfisher87 is that your latest? If so, let's get this merged!

Yes, looks like GitHub got caught up now :) Sounds great! You're good with using selective ignores to work around the places the tools disagree?

Also, @mfisher87, would you like to prepare a PR with a commit reverting 95f64ea?

Sure! Just to make sure I understand, this would be preparing a PR in advance to be merged if/when contributors feel more comfortable with Ruff?

mfisher87 commented 1 year ago

I felt the # isort: skip_file comment needed a bit more context, so I pushed another commit. If isort is removed in the future, it might not be clear that this comment corresponds to the isort library, but has no effect on ruff's isort ruleset (ruff skips look different). I wanted it to be really clear when it can be safely removed.

maresb commented 1 year ago

and in less than 30 minutes!

Ya, it was only timing out since there was a particular test that was hanging with Mamba, #483. Hopefully I can close it now!

You're good with using selective ignores to work around the places the tools disagree?

I didn't quite follow this. Are you talking about pre-commit? If so, I didn't set up the ignores for this repo.

Just to make sure I understand, this would be preparing a PR in advance to be merged if/when contributors feel more comfortable with Ruff?

Exactly.

mfisher87 commented 1 year ago

You're good with using selective ignores to work around the places the tools disagree?

I didn't quite follow this. Are you talking about pre-commit? If so, I didn't set up the ignores for this repo.

I was referring to the new ignores added in this PR - this and this. These are a bit hacky, but since we're going to have a reversion PR, we don't need to remember to come back and get rid of them later :)

maresb commented 1 year ago

Ya, no worries, that's fine.

conda / conda-lock