Skip to content

Conversation

@dylwil3
Copy link
Collaborator

@dylwil3 dylwil3 commented Apr 26, 2025

This PR promotes the fix applicability of readlines-in-for (FURB129) to always safe.

In the original PR (#9880), the author marked the rule as unsafe because Ruff's type inference couldn't quite guarantee that we had an IOBase object in hand. Some false positives were recorded in the test fixture. However, before the PR was merged, Charlie added the necessary type inference and the false positives went away.

According to the Python documentation, I believe this fix is safe for any proper implementation of IOBase:

IOBase (and its subclasses) supports the iterator protocol, meaning that an IOBase object can be iterated over yielding the lines in a stream. Lines are defined slightly differently depending on whether the stream is a binary stream (yielding bytes), or a text stream (yielding character strings). See readline() below.

and then in the documentation for readlines:

Read and return a list of lines from the stream. hint can be specified to control the number of lines read: no more lines will be read if the total size (in bytes/characters) of all lines so far exceeds hint. [...]
Note that it’s already possible to iterate on file objects using for line in file: ... without calling file.readlines().

I believe that a careful reading of our versioning policy requires that this change be deferred to a minor release - but please correct me if I'm wrong!

@dylwil3 dylwil3 added breaking Breaking API change fixes Related to suggested fixes for violations do-not-merge Do not merge this pull request labels Apr 26, 2025
@dylwil3 dylwil3 added this to the v0.12 milestone Apr 26, 2025
@dylwil3 dylwil3 changed the title [refurb] Mark fix as safe for readlines-in-for (FURB129) [refurb] Mark fix as safe for readlines-in-for (FURB129) Apr 26, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 26, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -0 violations, +10 -0 fixes in 1 projects; 54 projects unchanged)

apache/airflow (+0 -0 violations, +10 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

- dev/airflow_perf/sql_queries.py:145:41: FURB129 Instead of calling `readlines()`, iterate over file object directly
+ dev/airflow_perf/sql_queries.py:145:41: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
- dev/breeze/src/airflow_breeze/global_constants.py:553:21: FURB129 Instead of calling `readlines()`, iterate over file object directly
+ dev/breeze/src/airflow_breeze/global_constants.py:553:21: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
- devel-common/src/sphinx_exts/redirects.py:44:21: FURB129 Instead of calling `readlines()`, iterate over file object directly
+ devel-common/src/sphinx_exts/redirects.py:44:21: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
- providers/google/tests/system/google/cloud/gcs/resources/transform_script.py:26:39: FURB129 Instead of calling `readlines()`, iterate over file object directly
+ providers/google/tests/system/google/cloud/gcs/resources/transform_script.py:26:39: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly
- scripts/ci/pre_commit/newsfragments.py:36:43: FURB129 Instead of calling `readlines()`, iterate over file object directly
+ scripts/ci/pre_commit/newsfragments.py:36:43: FURB129 [*] Instead of calling `readlines()`, iterate over file object directly

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
FURB129 10 0 0 10 0

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

This sounds reasonable to me. Yes, I think this needs to be gated behind preview

@dylwil3 dylwil3 added preview Related to preview mode features and removed breaking Breaking API change do-not-merge Do not merge this pull request labels Apr 28, 2025
@dylwil3 dylwil3 removed this from the v0.12 milestone Apr 28, 2025
@dylwil3 dylwil3 merged commit 1e8881f into astral-sh:main Apr 28, 2025
33 checks passed
dcreager added a commit that referenced this pull request Apr 28, 2025
* main: (37 commits)
  [red-knot] Revert blanket `clippy::too_many_arguments` allow (#17688)
  Add config option to disable `typing_extensions` imports  (#17611)
  ruff_db: render file paths in diagnostics as relative paths if possible
  Bump mypy_primer pin (#17685)
  red_knot_python_semantic: improve `not-iterable` diagnostic
  [red-knot] Allow all callables to be assignable to @Todo-signatures (#17680)
  [`refurb`] Mark fix as safe for `readlines-in-for` (`FURB129`) (#17644)
  Collect preview lint behaviors in separate module (#17646)
  Upgrade Salsa to a more recent commit (#17678)
  [red-knot] TypedDict: No errors for introspection dunder attributes (#17677)
  [`flake8-pyi`] Ensure `Literal[None,] | Literal[None,]` is not autofixed to `None | None` (`PYI061`) (#17659)
  [red-knot] No errors for definitions of `TypedDict`s (#17674)
  Update actions/download-artifact digest to d3f86a1 (#17664)
  [red-knot] Use 101 exit code when there's at least one diagnostic with severity 'fatal' (#17640)
  [`pycodestyle`] Fix duplicated diagnostic in `E712` (#17651)
  [airflow] fix typos `AIR312` (#17673)
  [red-knot] Don't ignore hidden files by default (#17655)
  Update pre-commit hook astral-sh/ruff-pre-commit to v0.11.7 (#17670)
  Update docker/build-push-action digest to 14487ce (#17665)
  Update taiki-e/install-action digest to ab3728c (#17666)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fixes Related to suggested fixes for violations preview Related to preview mode features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants