[pycodestyle] Fix: Don't autofix if the first line ends in a question mark? (D400)

[yahayaohinoyi]

Summary

Fixes #13365

The autofix for D400 currently changes this:

def should_frob() -> bool:
    """Should we frob the bar?"""

To this:

def should_frob() -> bool:
    """Should we frob the bar?."""

Which seems pretty clearly worse! We probably just shouldn't offer an autofix if the first line ends in a question mark or exclamation mark?

DESCRIPTION

Basically, we don't want a diagnostic when we don't set a fix for this rule

ruff/crates/ruff_linter/src/rules/pydocstyle/rules/ends_with_period.rs

Line 113 in 8b3da18

checker.diagnostics.push(diagnostic);

.

Test Plan

Holistically followed the instructions here https://docs.astral.sh/ruff/contributing/#rule-testing-fixtures-and-snapshots

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[AlexWaygood]

Thanks for the PR! I think we should still emit a diagnostic if the first line ends in a question mark or exclamation mark, though, since according to PEP 257 the docstring isn't following the conventional style if it doesn't end in a period. We just shouldn't offer an autofix for the diagnostic if it ends in a question mark or exclamation mark, since we don't know what the appropriate autofix would be in that situation.

I left some inline suggestions below that should be sufficient to address this -- though you'll need to regenerate the snapshots again!

[AlexWaygood]

Suggested change

	checker.diagnostics.push(diagnostic);
	}
	}
	checker.diagnostics.push(diagnostic);

[MichaReiser]

I think we still need to move the checker.diagnostic.push call outside the trimmed.ends_with branch or we omit diagnostics for docstring summaries that end in !. We actually see this because I would expect to see more diagnostics in ruff_linter__rules__pydocstyle__tests__D415_D415.py.snap

[yahayaohinoyi]

I think we still need to move the checker.diagnostic.push call outside the trimmed.ends_with branch or we omit diagnostics for docstring summaries that end in !. We actually see this because I would expect to see more diagnostics in ruff_linter__rules__pydocstyle__tests__D415_D415.py.snap

Does it look any better now?

[MichaReiser]

See @AlexWaygood's comments

[yahayaohinoyi]

Need some help!. I get failures like
failures: rules::pydocstyle::tests::rules::rule_endsinperiod_path_new_d_py_expects
after running the tests. How do I get visibility of what exactly fails? Even when I remove the content of the whole file the test still fails. Also doesn't generate any snapshots

[MichaReiser]

Running the test gives me:

cargo test rules::pydocstyle::tests::rules::rule_endsinperiod_path_new_d_py_expects -p ruff_linter
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.08s
     Running unittests src/lib.rs (target/debug/deps/ruff_linter-93716c3568a00bf4)

running 1 test
test rules::pydocstyle::tests::rules::rule_endsinperiod_path_new_d_py_expects ... FAILED

failures:

---- rules::pydocstyle::tests::rules::rule_endsinperiod_path_new_d_py_expects stdout ----
thread 'rules::pydocstyle::tests::rules::rule_endsinperiod_path_new_d_py_expects' panicked at crates/ruff_linter/src/test.rs:254:21:
Rule EndsInPeriod is marked to always-fixable but the diagnostic has no fix.
Either ensure you always emit a fix or change `Violation::FIX_AVAILABILITY` to either `FixAvailability::Sometimes` or `FixAvailability::None`
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    rules::pydocstyle::tests::rules::rule_endsinperiod_path_new_d_py_expects

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 2108 filtered out; finished in 0.01s

The important bits are

thread 'rules::pydocstyle::tests::rules::rule_endsinperiod_path_new_d_py_expects' panicked at crates/ruff_linter/src/test.rs:254:21:
Rule EndsInPeriod is marked to always-fixable but the diagnostic has no fix.
Either ensure you always emit a fix or change `Violation::FIX_AVAILABILITY` to either `FixAvailability::Sometimes` or `FixAvailability::None`

It's telling you that the Violation should no longer implement AlwaysFixableViolation and instead explicitly set the Fixability to Sometimes

impl Violation for EndsInPeriod {
    /// `None` in the case a fix is never available or otherwise Some
    /// [`FixAvailability`] describing the available fix.
    const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

    #[derive_message_formats]
    fn message(&self) -> String {
        format!("First line should end with a period")
    }

    fn fix_title(&self) -> String {
        "Add period".to_string()
    }
}

[MichaReiser]

Nice find

[MichaReiser]

Thank you