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

mfisher87 · 2023-08-28T00:42:09Z

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 · 2023-08-28T00:42:14Z

✅ Deploy Preview for conda-lock ready!

Name	Link
🔨 Latest commit	`52cb31b`
🔍 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 · 2023-08-28T00:49:52Z

pyproject.toml

+    "E501",
+    "F401",
+    "F403",
+    # Disabled during migration to Ruff:


Many of these can be enabled and autofixed, but I felt it would be better to tackle it in a separate PR to keep a reasonable scope for this one :)

mfisher87 · 2023-08-28T00:54:44Z

pyproject.toml

+select = [
+    "A",  # flake8-builtins
+    # "B",  # flake8-bugbear
+    "B006",  # Do not use mutable data structures for argument defaults


flake8-mutable errors are caught by this rule. Looks to have detected something that was previously undetected, too.

Ruff is fast, and this commit only aims to take advange of that, not to add new rules.

These all felt likely to be uncontroversial choices for letting Ruff autofix some new rules.

mfisher87 · 2023-08-28T14:42:06Z

PR got autoclosed because of a rebase mistake, woops.

mfisher87 · 2023-08-28T18:26:16Z

conda_lock/conda_lock.py

@@ -768,7 +768,7 @@ def create_lockfile_from_spec(
    *,
    conda: PathLike,
    spec: LockSpecification,
-    platforms: List[str] = [],
+    platforms: Optional[List[str]] = None,


Thinking out loud: This is probably safe as-is because this function would only be called one time per CLI invocation. But if that assumption changes, this use of a mutable default value could bite in the future.

mfisher87 · 2023-08-28T18:27:37Z

requirements-dev.txt

@@ -1,19 +1,12 @@
 black


If black is installed and run with pre-commit, maybe it doesn't need to also be here? Same for mypy?

It's still useful to have in the dev environment. If you're running VS Code, then black is a requirement for "Format Document..." (although I'm not 100% sure this is still the case since they came out with the Black VS Code extension.)

Also, ruff is a linter, not a type checker, so I think we want to keep mypy, at least for now.

Also, ruff is a linter, not a type checker, so I think we want to keep mypy, at least for now.

Yes, I was more thinking about mypy being on the slow side to use in a pre-commit workflow, not about eliminating it entirely. I don't want to touch mypy for this PR in any case, one thing at a time is enough for me 😆

It's still useful to have in the dev environment. If you're running VS Code, then black is a requirement for "Format Document..." (although I'm not 100% sure this is still the case since they came out with the Black VS Code extension.)

Interesting. I'm not a VSCode user, but I assumed that would integrate with pre-commit as well. Makes sense!

maresb

Thanks a lot for this!

I understand mypy being slow, but I've never found flake8 to be cumbersome. Maybe I work more slowly than most people.

I know lots of people are really excited about ruff, and it is incredibly fast. On the other hand, flake8 is tried-and-true, and I like its extensibility with plugins.

How would you feel about leaving in flake8 and isort for now? In my current main project I added ruff at the end of my pre-commit list. That was a mistake because I never see it do anything. Let's do better here and put it at the top of the list, and then if I see flake8 no longer doing anything then I'll learn to trust it.

maresb · 2023-08-28T23:43:33Z

requirements-dev.txt

@@ -1,19 +1,12 @@
 black


Also, ruff is a linter, not a type checker, so I think we want to keep mypy, at least for now.

mfisher87 · 2023-08-29T00:58:37Z

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 · 2023-08-29T01:26:13Z

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

I'm sure there are better ways to do this than `# isort: skip_file` and flake8 `per-file-ignores`, but this is what I had time for today :)

mfisher87 · 2023-08-29T02:02:02Z

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 · 2023-08-29T03:37:44Z

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 · 2023-08-29T03:42:36Z

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 · 2023-08-30T00:17:09Z

Since when is the Windows test passing?

🤩 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 · 2023-08-30T00:24:56Z

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 · 2023-08-30T00:32:36Z

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 · 2023-08-30T01:31:37Z

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 :)

In conda#490, Ruff was added to try it out. Ruff could theoretically replace isort and flake8. This commit removes them and removes compatibility hacks in case we decide we do want to drop isort and flake8.

maresb · 2023-08-30T02:51:56Z

Ya, no worries, that's fine.

mfisher87 requested a review from a team as a code owner August 28, 2023 00:42

mfisher87 commented Aug 28, 2023

View reviewed changes

mfisher87 closed this Aug 28, 2023

mfisher87 force-pushed the ruff branch from d73d945 to 1758dc7 Compare August 28, 2023 14:31

mfisher87 added 4 commits August 28, 2023 08:40

Switch from flake8 (and isort) to ruff

e21db50

Ruff is fast, and this commit only aims to take advange of that, not to add new rules.

Autofix a manageable set of new rules

f015fdd

These all felt likely to be uncontroversial choices for letting Ruff autofix some new rules.

Specify target version

3aca73e

Enable and autofix B006

dbd131e

mfisher87 reopened this Aug 28, 2023

mfisher87 commented Aug 28, 2023

View reviewed changes

maresb reviewed Aug 28, 2023

View reviewed changes

Restore flake8 and isort in a way that's compatible with ruff

95f64ea

I'm sure there are better ways to do this than `# isort: skip_file` and flake8 `per-file-ignores`, but this is what I had time for today :)

maresb approved these changes Aug 29, 2023

View reviewed changes

mfisher87 changed the title ~~Replace flake8 and isort with Ruff~~ Add Ruff to pre-commit (before isort + flake8 to test it as a possible replacement) Aug 30, 2023

Add context to isort skip comment

52cb31b

maresb merged commit 2f075e7 into conda:main Aug 30, 2023
9 checks passed

mfisher87 mentioned this pull request Aug 30, 2023

Remove flake8 and isort #493

Merged

mfisher87 deleted the ruff branch August 30, 2023 01:41

maresb mentioned this pull request Oct 17, 2023

Add more pre-commit hooks #420

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mfisher87 commented Aug 28, 2023

netlify bot commented Aug 28, 2023 •

edited

Loading

mfisher87 Aug 28, 2023

mfisher87 Aug 28, 2023

mfisher87 commented Aug 28, 2023

mfisher87 Aug 28, 2023

mfisher87 Aug 28, 2023

maresb Aug 28, 2023

maresb Aug 28, 2023

mfisher87 Aug 29, 2023

mfisher87 Aug 29, 2023

maresb left a comment

maresb Aug 28, 2023

mfisher87 commented Aug 29, 2023

mfisher87 commented Aug 29, 2023

mfisher87 commented Aug 29, 2023

maresb commented Aug 29, 2023

maresb commented Aug 29, 2023

mfisher87 commented Aug 30, 2023

mfisher87 commented Aug 30, 2023

maresb commented Aug 30, 2023

mfisher87 commented Aug 30, 2023

maresb commented Aug 30, 2023

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

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

Conversation

mfisher87 commented Aug 28, 2023

Description

netlify bot commented Aug 28, 2023 • edited Loading

✅ Deploy Preview for conda-lock ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfisher87 commented Aug 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maresb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfisher87 commented Aug 29, 2023

mfisher87 commented Aug 29, 2023

mfisher87 commented Aug 29, 2023

maresb commented Aug 29, 2023

maresb commented Aug 29, 2023

mfisher87 commented Aug 30, 2023

mfisher87 commented Aug 30, 2023

maresb commented Aug 30, 2023

mfisher87 commented Aug 30, 2023

maresb commented Aug 30, 2023

netlify bot commented Aug 28, 2023 •

edited

Loading