Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use ruff --preview, drop flake8 #20318

Merged
merged 2 commits into from
Apr 16, 2024
Merged

Use ruff --preview, drop flake8 #20318

merged 2 commits into from
Apr 16, 2024

Conversation

allisonkarlitskaya
Copy link
Member

No description provided.

@allisonkarlitskaya allisonkarlitskaya changed the title Unflake Use ruff --preview, drop flake8 Apr 15, 2024
martinpitt
martinpitt requested changes Apr 16, 2024
View reviewed changes
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like!

test/common/testlib.py Outdated Show resolved Hide resolved
@martinpitt
Copy link
Member

Looks good in general, but a sea of TypeError: 'NoneType' object is not callable

@allisonkarlitskaya
Copy link
Member Author

Looks good in general, but a sea of TypeError: 'NoneType' object is not callable

I think I was too tired when I did this 😅

@allisonkarlitskaya
pyproject: enable 'preview' ruff rules
be0bfa8 
Now that we gate our ruff updates via the tasks container, we can afford
to be a bit more future-oriented here.

This brings in a nice raft of fix-ups, including my personal favourite:

  RUF003 Comment contains ambiguous `µ` (MICRO SIGN). Did you mean `μ` (GREEK SMALL LETTER MU)?

Sure... why not?

The majority of fixes here are from a single rule suggesting to rewrite

  a and b or c

as

  (a and b) or c

But in most (but not all) cases in our code we actually want to write
that as

  b if a else c

I've also added some type annotations to help me make the right choice
in places where it wasn't immediately clear.
@allisonkarlitskaya
test/static-code: drop flake8
760ac17 
`ruff` in preview mode now implements the various E[123] rules that we
were depending on flake8 for.  Drop it.
@jelly
Copy link
Member

jelly commented Apr 16, 2024 •
edited
Loading

Running with ruff --preview means we will get unstable rules by default, means we will get more work and become beta testers. Is that really what we want? Also the preview ruleset is likely to change every release becoming quite some churn.

Is it known when the rules become "stable"? Is it a month, a week? I couldn't figure that out when I was looking into it.

martinpitt
martinpitt approved these changes Apr 16, 2024
View reviewed changes
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with this, I figure my taste of "fix future complains ealier" differs from Jelle's. Definitively don't want to override you there, just retracting my -1.

@allisonkarlitskaya
Copy link
Member Author

Running with ruff --preview means we will get unstable rules by default, means we will get more work and become beta testers. Is that really what we want? Also the preview ruleset is likely to change every release becoming quite some churn.

My take: probably like 95% of rules that end up in --preview get promoted to stable at some point, so that's work we'd end up having to do anyway... and as a project, we'll well positioned. We gate our task container updates, so we get a chance to fix things cleanly on incoming new rules, and at least a couple of us have even contributed to ruff already... and it gives us a chance to have input upstream on new rules before they go stable.

If we end up seeing something we don't like we always have the option to mask out that new rule and argue against it upstream, before it lands in stable...

Is it known when the rules become "stable"? Is it a month, a week? I couldn't figure that out when I was looking into it.

Unfortunately, ruff seems a bit opaque on this point. There's https://docs.astral.sh/ruff/preview/ but it doesn't say much about the lifecycle/trajectory of a preview rule...

@jelly jelly merged commit 6298dd9 into cockpit-project:main Apr 16, 2024
76 checks passed
@jelly
Copy link
Member

jelly commented Apr 16, 2024

Merged, I'll handle the files flake8 removal.

@martinpitt martinpitt deleted the unflake branch April 16, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinpitt martinpitt martinpitt approved these changes

@jelly jelly jelly approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@allisonkarlitskaya @martinpitt @jelly