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.

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

chore: Enable more ruff rules #21043

Merged
merged 1 commit into from
Apr 8, 2024
Merged

chore: Enable more ruff rules #21043

merged 1 commit into from
Apr 8, 2024

Conversation

webjunkie
Copy link
Contributor

Problem

We can remove the ignore of some rules that can be autofixed or that we don't break anyways.

Changes

Remove ignore for these rules

Details

assert-false (B011)

Derived from the flake8-bugbear linter.

Fix is always available.

What it does

Checks for uses of assert False.

Why is this bad?

Python removes assert statements when running in optimized mode
(python -O), making assert False an unreliable means of
raising an AssertionError.

Instead, raise an AssertionError directly.

Example

assert False

Use instead:

raise AssertionError

References

abstract-base-class-without-abstract-method (B024)

Derived from the flake8-bugbear linter.

What it does

Checks for abstract classes without abstract methods.

Why is this bad?

Abstract base classes are used to define interfaces. If they have no abstract
methods, they are not useful.

If the class is not meant to be used as an interface, it should not be an
abstract base class. Remove the ABC base class from the class definition,
or add an abstract method to the class.

Example

from abc import ABC


class Foo(ABC):
    def method(self):
        bar()

Use instead:

from abc import ABC, abstractmethod


class Foo(ABC):
    @abstractmethod
    def method(self):
        bar()

References

unnecessary-generator-set (C401)

Derived from the flake8-comprehensions linter.

Fix is always available.

What it does

Checks for unnecessary generators that can be rewritten as set
comprehensions.

Why is this bad?

It is unnecessary to use set around a generator expression, since
there are equivalent comprehensions for these types. Using a
comprehension is clearer and more idiomatic.

Examples

set(f(x) for x in foo)

Use instead:

{f(x) for x in foo}

unnecessary-collection-call (C408)

Derived from the flake8-comprehensions linter.

Fix is always available.

What it does

Checks for unnecessary dict, list or tuple calls that can be
rewritten as empty literals.

Why is this bad?

It's unnecessary to call e.g., dict() as opposed to using an empty
literal ({}). The former is slower because the name dict must be
looked up in the global scope in case it has been rebound.

Examples

dict()
dict(a=1, b=2)
list()
tuple()

Use instead:

{}
{"a": 1, "b": 2}
[]
()

unnecessary-call-around-sorted (C413)

Derived from the flake8-comprehensions linter.

Fix is always available.

What it does

Checks for unnecessary list or reversed calls around sorted
calls.

Why is this bad?

It is unnecessary to use list around sorted, as the latter already
returns a list.

It is also unnecessary to use reversed around sorted, as the latter
has a reverse argument that can be used in lieu of an additional
reversed call.

In both cases, it's clearer to avoid the redundant call.

Examples

reversed(sorted(iterable))

Use instead:

sorted(iterable, reverse=True)

unnecessary-double-cast-or-process (C414)

Derived from the flake8-comprehensions linter.

Fix is always available.

What it does

Checks for unnecessary list, reversed, set, sorted, and tuple
call within list, set, sorted, and tuple calls.

Why is this bad?

It's unnecessary to double-cast or double-process iterables by wrapping
the listed functions within an additional list, set, sorted, or
tuple call. Doing so is redundant and can be confusing for readers.

Examples

list(tuple(iterable))

Use instead:

list(iterable)

This rule applies to a variety of functions, including list, reversed,
set, sorted, and tuple. For example:

  • Instead of list(list(iterable)), use list(iterable).
  • Instead of list(tuple(iterable)), use list(iterable).
  • Instead of tuple(list(iterable)), use tuple(iterable).
  • Instead of tuple(tuple(iterable)), use tuple(iterable).
  • Instead of set(set(iterable)), use set(iterable).
  • Instead of set(list(iterable)), use set(iterable).
  • Instead of set(tuple(iterable)), use set(iterable).
  • Instead of set(sorted(iterable)), use set(iterable).
  • Instead of set(reversed(iterable)), use set(iterable).
  • Instead of sorted(list(iterable)), use sorted(iterable).
  • Instead of sorted(tuple(iterable)), use sorted(iterable).
  • Instead of sorted(sorted(iterable)), use sorted(iterable).
  • Instead of sorted(reversed(iterable)), use sorted(iterable).

unnecessary-comprehension (C416)

Derived from the flake8-comprehensions linter.

Fix is always available.

What it does

Checks for unnecessary dict, list, and set comprehension.

Why is this bad?

It's unnecessary to use a dict/list/set comprehension to build a
data structure if the elements are unchanged. Wrap the iterable with
dict(), list(), or set() instead.

Examples

{a: b for a, b in iterable}
[x for x in iterable]
{x for x in iterable}

Use instead:

dict(iterable)
list(iterable)
set(iterable)

unnecessary-map (C417)

Derived from the flake8-comprehensions linter.

Fix is sometimes available.

What it does

Checks for unnecessary map calls with lambda functions.

Why is this bad?

Using map(func, iterable) when func is a lambda is slower than
using a generator expression or a comprehension, as the latter approach
avoids the function call overhead, in addition to being more readable.

Examples

map(lambda x: x + 1, iterable)

Use instead:

(x + 1 for x in iterable)

This rule also applies to map calls within list, set, and dict
calls. For example:

  • Instead of list(map(lambda num: num * 2, nums)), use
    [num * 2 for num in nums].
  • Instead of set(map(lambda num: num % 2 == 0, nums)), use
    {num % 2 == 0 for num in nums}.
  • Instead of dict(map(lambda v: (v, v ** 2), values)), use
    {v: v ** 2 for v in values}.

Does this work well for both Cloud and self-hosted?

No impact

How did you test this code?

  • code should work as before

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@webjunkie webjunkie removed the stale label Apr 3, 2024
@webjunkie webjunkie requested a review from a team April 4, 2024 10:32
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

The code looks good from a scroll through, but I don't think we can merge such an increase in the mypy baseline

Copy link
Member

Choose a reason for hiding this comment

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

That's a LOT of new mypy errors, it's hard to be confident that none are legit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, there are many. Almost all seem to be the result of the change of default values to explicit None and since the types didn't get adjusted (missing Optional[xyz]) these come up. I'll look if there's a fix or leave this category out completely and handle it separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No autofix found, delegating to the future.

@webjunkie webjunkie requested a review from Twixes April 8, 2024 14:19
@webjunkie webjunkie merged commit 487ca39 into master Apr 8, 2024
77 checks passed
@webjunkie webjunkie deleted the chore/ruff-rules branch April 8, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants