astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
31.95k stars 1.07k forks source link

Rule idea: Prefer dict merge #13706

Open jaap3 opened 1 week ago

jaap3 commented 1 week ago

I just made a PR in one of our private projects replacing patterns like:

def get_form_kwargs(self):
    return {
        **super().get_form_kwargs(),
        "instance": self.request.user,
        "claims": self.token_data["claims"],
    }

and

def get_form_kwargs(self):
    kwargs = super().get_form_kwargs()
    kwargs.update(
        instance=self.request.user,
        claims=self.token_data["claims"]
    )
    return kwargs

with

def get_form_kwargs(self):
    return super().get_form_kwargs() | {
        "instance": self.request.user,
        "claims": self.token_data["claims"],
    }

Then I thought to myself, it would be nice if Ruff could just autofix this.

I looked into the rules, and open issues and didn't find an existing rule covering this.

I did come across https://github.com/astral-sh/ruff/issues/13533#issuecomment-2394953512 which I think is suggesting (nearly) the same thing.

dylwil3 commented 1 week ago

In the context of lists, there's already a lint rule like this but it suggests the opposite: RUF005

jaap3 commented 3 days ago

I don't actually particularly care about the exact form of how dictionaries are merged, it's just that I'd like it to be consistent across the codebase. I picked the union operator | because it's a recent language addition and resulted in the lowest line count in my case.

Just did a quick (unscientific) benchmark and it appears unpacking is currently more performant:

python3.13 -m timeit -s "a = {'a': 1};b = {'b': 2};c = {'a': 0}" "m = a | b | c"
2000000 loops, best of 5: 118 nsec per loop

python3.13 -m timeit -s "a = {'a': 1};b = {'b': 2};c = {'a': 0}" "m = {**a, **b, **c}"
5000000 loops, best of 5: 86.3 nsec per loop

Opinions may vary on the readability of the latter, but I'm fine with either.

jaap3 commented 2 days ago

Ran into an interesting variation that combined both unpacking and the union operator:

def get_form_kwargs(self):
    return {**super().get_form_kwargs()} | {
        "instance": self.request.user,
        "claims": self.token_data["claims"],
    }

Adding it here as a reminder to also detect this pattern if a rule is implemented.

lengau commented 6 hours ago

I like this idea, but I'd prefer consistency with RUF005 and have dict unpacking.