Skip to content

Comments

Refactor tab-indentation as a token-based rule#7435

Merged
dhruvmanila merged 3 commits intomainfrom
dhruv/tab-indentation-using-tok
Sep 16, 2023
Merged

Refactor tab-indentation as a token-based rule#7435
dhruvmanila merged 3 commits intomainfrom
dhruv/tab-indentation-using-tok

Conversation

@dhruvmanila
Copy link
Member

Summary

This PR updates the W191 (tab-indentation) rule from a line-based to a
token-based rule.

Earlier, the rule used the triple_quoted_string_ranges from the indexer to
skip over any lines inside a triple-quoted string. This was the only use of
the ranges. These ranges were extracted through the tokens, so instead we can
directly use the newline tokens to perform the check.

This would also mean that we can remove the triple_quoted_string_ranges from
the indexer but I'll hold that off until we have a better idea on #7326 but I
don't think it would be a problem to remove it.

This will also fix #7379 once PEP 701 changes are merged.

Test Plan

cargo test

@dhruvmanila
Copy link
Member Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@dhruvmanila dhruvmanila added the internal An internal refactor or improvement label Sep 16, 2023
line_start: TextSize,
) {
let indent = leading_indentation(locator.after(line_start));
if indent.find('\t').is_some() {
Copy link
Member

Choose a reason for hiding this comment

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

We could probably do the leading_indentation and tab find in one pass, via take_while and find?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried this but we need the indent length for the diagnostic range:

TextRange::at(line_start, indent.text_len())

@github-actions
Copy link
Contributor

github-actions bot commented Sep 16, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

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.

Nice

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 16, 2023

CodSpeed Performance Report

Merging #7435 will not alter performance

Comparing dhruv/tab-indentation-using-tok (c025c41) with main (422ff82)

Summary

✅ 25 untouched benchmarks

@dhruvmanila dhruvmanila force-pushed the dhruv/tab-indentation-using-tok branch from 1245af9 to c025c41 Compare September 16, 2023 20:03
@dhruvmanila dhruvmanila enabled auto-merge (squash) September 16, 2023 20:05
auto-merge was automatically disabled September 16, 2023 20:16

Pull request was closed

@dhruvmanila dhruvmanila reopened this Sep 16, 2023
@dhruvmanila dhruvmanila enabled auto-merge (squash) September 16, 2023 20:17
@dhruvmanila dhruvmanila merged commit 959338d into main Sep 16, 2023
@dhruvmanila dhruvmanila deleted the dhruv/tab-indentation-using-tok branch September 16, 2023 20:25
dhruvmanila added a commit that referenced this pull request Sep 18, 2023
## Summary

This is a follow-up PR for #7435 to remove the now unused triple-quoted
string ranges from the indexer.
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.

W191 isn't triggered inside multi-line f-string expression

3 participants