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 for D413 introduces whitespace #10050

Closed
jhossbach opened this issue Feb 19, 2024 · 3 comments · Fixed by #10162
Closed

Fix for D413 introduces whitespace #10050

jhossbach opened this issue Feb 19, 2024 · 3 comments · Fixed by #10162
Labels
bug Something isn't working docstring Related to docstring linting or formatting fixes Related to suggested fixes for violations

Comments

@jhossbach
Copy link

Minimal test.py:

def test_func():
    """Some test function.

    Returns
    -------
    None
    """
    pass

Fixing with ruff --isolated --select="D413" --fix test.py adds unnecessary whitespace (W293)

--- a/test.py
+++ b/test.py
@@ -4,5 +4,6 @@ def test_func():
     Returns
     -------
     None
+    
     """
     pass
@AlexWaygood AlexWaygood added bug Something isn't working docstring Related to docstring linting or formatting fixes Related to suggested fixes for violations labels Feb 19, 2024
@jusexton
Copy link
Contributor

jusexton commented Feb 21, 2024

I took a quick look at this and the culprit seems to be here:

if num_blank_lines < 2 {
let mut diagnostic = Diagnostic::new(
BlankLineAfterLastSection {
name: context.section_name().to_string(),
},
docstring.range(),
);
// Add a newline after the section.
diagnostic.set_fix(Fix::safe_edit(Edit::insertion(
format!(
"{}{}",
line_end.repeat(2 - num_blank_lines),
docstring.indentation
),
context.end(),
)));
checker.diagnostics.push(diagnostic);

Instead of the fix "introducing" whitespace it is more so leaving behind the original indentation space which obviously violates (W293). In this scenario I would imagine the original indentation will need to be deleted before inserting the new line and new indentation. Below is an initial attempt at addressing this problem. Let me know if this is a good solution and I would be glad to submit a pull request.

let fix = match num_blank_lines {
    0 => Fix::safe_edit(Edit::insertion(
        format!("{}{}", line_end.repeat(2), docstring.indentation),
        context.end(),
    )),
    1 => {
        let del_start = context.end() - TextSize::from(docstring.indentation.len() as u32);
        let rest = [Edit::insertion(
            format!("{}{}", line_end, docstring.indentation),
            context.end(),
        )];
        Fix::safe_edits(Edit::deletion(del_start, context.end()), rest)
    }
    _ => unreachable!(),
};
diagnostic.set_fix(fix);

@MichaReiser
Copy link
Member

@jusexton this looks about right, although it would be nice if we don't have to branch on the existing empty lines. I also think that this won't work if the line has more whitespace than just the indent?

def test_func():
    """Some test function.

    Returns
    -------
    None
           """
    pass

Could we maybe use the range information in context.following_lines() to determine the offset?

@jusexton
Copy link
Contributor

@MichaReiser Thanks for the help. A PR has been created making use of context.following_lines() to determine the deletion length instead of docstring.indentation. I also attempted removing all branching but a check still seems to be required around whether the last line is whitespace or not. Let me know if there is a better way!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working docstring Related to docstring linting or formatting fixes Related to suggested fixes for violations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants