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

Solve ruff issue non-augmented assignment #8296

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

berland
Copy link
Contributor

@berland berland commented Jul 3, 2024

Issue
Resolves ruff issue on non-augmented assignment

Approach
ruff check --unsafe-fixes --fix ., 👀

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure tests pass locally (after every commit!)

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Create Backport PR to latest release

@berland
Copy link
Contributor Author

berland commented Jul 3, 2024

non-augmented-assignment (PLR6104)

Derived from the Pylint linter.

Fix is always available.

This rule is in preview and is not stable. The --preview flag is required for use.

What it does

Checks for assignments that can be replaced with augmented assignment
statements.

Why is this bad?

If an assignment statement consists of a binary operation in which one
operand is the same as the assignment target, it can be rewritten as an
augmented assignment. For example, x = x + 1 can be rewritten as
x += 1.

When performing such an operation, augmented assignments are more concise
and idiomatic.

Known problems

In some cases, this rule will not detect assignments in which the target
is on the right-hand side of a binary operation (e.g., x = y + x, as
opposed to x = x + y), as such operations are not commutative for
certain data types, like strings.

For example, x = "prefix-" + x is not equivalent to x += "prefix-",
while x = 1 + x is equivalent to x += 1.

If the type of the left-hand side cannot be inferred trivially, the rule
will ignore the assignment.

Example

x = x + 1

Use instead:

x += 1

Fix safety

This rule's fix is marked as unsafe, as augmented assignments have
different semantics when the target is a mutable data type, like a list or
dictionary.

For example, consider the following:

foo = [1]
bar = foo
foo = foo + [2]
assert (foo, bar) == ([1, 2], [1])

If the assignment is replaced with an augmented assignment, the update
operation will apply to both foo and bar, as they refer to the same
object:

foo = [1]
bar = foo
foo += [2]
assert (foo, bar) == ([1, 2], [1, 2])

@berland berland self-assigned this Jul 3, 2024
@berland berland added the release-notes:skip If there should be no mention of this in release notes label Jul 3, 2024
@berland berland enabled auto-merge (rebase) July 3, 2024 17:45
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.94%. Comparing base (6be9385) to head (f942034).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8296      +/-   ##
==========================================
+ Coverage   86.92%   86.94%   +0.01%     
==========================================
  Files         375      375              
  Lines       23523    23523              
  Branches      624      636      +12     
==========================================
+ Hits        20448    20452       +4     
+ Misses       2999     2998       -1     
+ Partials       76       73       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@JHolba JHolba left a comment

Choose a reason for hiding this comment

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

Nice changes.
The comment listing pitfalls was excellent :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:skip If there should be no mention of this in release notes
Projects
Status: Reviewed
Development

Successfully merging this pull request may close these issues.

None yet

3 participants