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

Fix indentation error when trailing quotes are not on separate line (… #13372

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ukyen8
Copy link
Contributor

@ukyen8 ukyen8 commented Sep 16, 2024

…D208)

Summary

This PR is to fix an unexpected change of the D208 rule when trailing quotes are not on a separate line.

Related issue: #13260

Test Plan

Add more test cases to D208.py and update the snapshot file.

Copy link
Contributor

github-actions bot commented Sep 16, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter 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.

Thanks. There are a few extra cases that we should consider but this is heading in a good direction

// When a non-space character is on the last line, the trailing quotes are also on the same line.
// No diagnostic is added if the indentation difference equals the indent size.
if last.chars().any(|c| !c.is_whitespace()) {
let prev_line_indent = leading_space(&lines[lines.len() - 2]);
Copy link
Member

Choose a reason for hiding this comment

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

I think this might panic if there are fewer than 3 lines because it then results in an out of bound access.

We should also consider the case where there's a number of blank lines between the last and previous line

def right_indent_quotes_same_line(a: int) -> None:
    """Foo.
    Parameters
    ----------
    a : int





        A parameter."""
    pass

let mut offset_len = line_indent.text_len();
// When a non-space character is on the last line, the trailing quotes are also on the same line.
// No diagnostic is added if the indentation difference equals the indent size.
if last.chars().any(|c| !c.is_whitespace()) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can simplify this by testing if the line_indent.text_len() == last.text_len()`. If the condition is true, then the last line is either all whitespace or empty

Comment on lines 299 to 303
let prev_line_indent_size = prev_line_indent.chars().count();
if line_indent_size - prev_line_indent_size == docstring_indent_size {
return;
}
offset_len = line_indent.text_len() - prev_line_indent.text_len();
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this, but could we possibly use over_indented_size?

@MichaReiser MichaReiser added bug Something isn't working rule Implementing or modifying a lint rule labels Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants