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

Extract LineIndex independent methods from Locator #13938

Merged
merged 2 commits into from
Oct 28, 2024
Merged

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Oct 26, 2024

Summary

The Locator provides useful methods to extract a line's range or text given an offset
or the full range or text of all lines falling into a text range.

These methods are foundational and used across many crates.

However, Locator also exposes methods that require computing and memoizing a LineIndex.
The formatter doesn't use any of those methods and "cheats" by always creating an ad-hoc Locator
instead of storing an instance on PyFormatContext. This leaks the Locator abstraction
because it depends on the knowledge of which Locator methods require the extra LineIndex state.

This PR splits the "stateless" methods from Locator into a new trait and implements that trait
for str (string slices). It also replaces all Locator usages with &str in all crates other than ruff_linter.
It also moves Locator to ruff_linter to clarify that this is linter-specific infrastructure.

I decided to keep Locator for ruff_linter because:

  • Locator communicates that this is the source of the entire file
  • Some linter code uses the LineIndex dependent Locator methods where the lazy index computation is useful.

We could consider changing more Linter methods to take &str instead of Locator but we can do this in separate PRs.

Renamed methods

I had to rename lines to lines_str str.lines conflicts with the std lines builtin (returns an iterator over the lines). I decided to add the _str postfix to all other methods that return a str for consistency.

Test Plan

cargo test

@MichaReiser MichaReiser added the internal An internal refactor or improvement label Oct 26, 2024
Copy link
Contributor

github-actions bot commented Oct 26, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Nice, this is great!

crates/red_knot_test/src/assertion.rs Outdated Show resolved Hide resolved
crates/ruff_formatter/src/builders.rs Outdated Show resolved Hide resolved
crates/ruff_text_size/src/traits.rs Show resolved Hide resolved
@MichaReiser MichaReiser enabled auto-merge (squash) October 28, 2024 07:48
@MichaReiser MichaReiser merged commit 9f3a38d into main Oct 28, 2024
18 checks passed
@MichaReiser MichaReiser deleted the micha/line-ranges branch October 28, 2024 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants