Skip to content

CLN: Enable ruff rule PLR1730 (if-stmt-min-max)#64130

Open
Moisan wants to merge 1 commit intopandas-dev:mainfrom
Moisan:CLN-enable-ruff-PLR1730
Open

CLN: Enable ruff rule PLR1730 (if-stmt-min-max)#64130
Moisan wants to merge 1 commit intopandas-dev:mainfrom
Moisan:CLN-enable-ruff-PLR1730

Conversation

@Moisan
Copy link
Contributor

@Moisan Moisan commented Feb 13, 2026

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.
  • I have reviewed and followed all the contribution guidelines
  • If I used AI to develop this pull request, I prompted it to follow AGENTS.md.

@Moisan Moisan requested a review from rhshadrach as a code owner February 13, 2026 01:16
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

To be honest, I am not sure I find this more readable or easier to understand ...

if is_max:
if val > output[lab]:
output[lab] = val
output[lab] = max(output[lab], val)
Copy link
Member

Choose a reason for hiding this comment

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

This is not actually equivalent? Because in the new code, the setitem operation always happens, while in the existing code only if needed (that can have a performance difference?)

Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

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

This linting rule is under intentionally disabled in pyproject.toml. If you think that is a wrong decision, open an issue for discussion.

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