Skip to content

Conversation

@DimitriPapadopoulos
Copy link
Contributor

@DimitriPapadopoulos DimitriPapadopoulos commented Nov 19, 2025

I am not enforcing these rules yet.

I've left out a bunch of rules.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 19, 2025

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

    27 files  ±0      27 suites  ±0   9h 50m 53s ⏱️ - 4m 24s
 4 112 tests ±0   4 005 ✅ +1    104 💤 ±0  3 ❌ ±0 
51 516 runs   - 1  49 329 ✅ ±0  2 184 💤 ±0  3 ❌ ±0 

For more details on these failures, see this check.

Results for commit 426420e. ± Comparison against base commit d55a2fd.

♻️ This comment has been updated with latest results.

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

There are some good readability and performance improvements in here thanks! Are you not enforcing because they require manually fixing? If they need manual work then I think we should not enforce them.

Personally I would only use next(iter(...)) in cases where performance matters as it's not so novice friendly. If we really want to get away from x = list(...)[0] we can always use [x, _*] = ... which can be easier for novices to parse than x = next(iter(...)).

I also think RUF010 reduces readability by using advanced f-string expressions. The original code was already very readable.

@DimitriPapadopoulos
Copy link
Contributor Author

I'm not enforcing the RUF rules because I haven't applied all of them. I was thinking of ignoring a bunch of them.

I agree that next(iter(...)) is not novice friendly. I didn't like it at first either. Would you like me to ignore it?

Finally, I modified manually the result of RUF010 and I think this changes does make sense:
f"{str(type(exc))}"f"{type(exc)}"
The other one might be debatable, but there are already 108 occurrences of {...!r} vs. this single occurrence of {repr(...)} in this repository!
f"{repr(name)}"f"{name!r}"
Therefore, I would recommend keeping RUF010, if only for consistency.

@jacobtomlinson
Copy link
Member

I agree that next(iter(...)) is not novice friendly. I didn't like it at first either. Would you like me to ignore it?

Yes sounds good

The other one might be debatable, but there are already 108 occurrences of {...!r} vs. this single occurrence of {repr(...)} in this repository!

Fair point. I think {...!r} is less readable than {repr(...)}, but if we already have it all over then let's go with it for consistency.

@DimitriPapadopoulos
Copy link
Contributor Author

Rebased + ignoring RUF015.

@DimitriPapadopoulos DimitriPapadopoulos changed the title Start applying ruff rules (RUF) Enforce ruff rules (RUF) Nov 25, 2025
RUF003 Comment contains ambiguous ` ` (NO-BREAK SPACE). Did you mean ` ` (SPACE)?
RUF007 Prefer `itertools.pairwise()` over `zip()` when iterating over successive pairs
RUF010 Use explicit conversion flag
RUF017 Avoid quadratic list summation
RUF046 Value being cast to `int` is already an integer
RUF051 Use `pop` instead of `key in dict` followed by `del dict[key]`
@jacobtomlinson jacobtomlinson merged commit 1a1953d into dask:main Nov 27, 2025
27 of 34 checks passed
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.

2 participants