[formatter] Fix trailing newline insertion in fmt: off regions

[danparizher]

Summary

Fixes #19492

[github-actions]

ruff-ecosystem results

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

[MichaReiser]

Thank you.

This will need some tests. Testing whether the source contains any #fmt: off is also not strict enough. E.g.

# fmt: off
a = 10
# fmt: on

The new line should be inserted for the case above

This might require integration into fmt_suite where we track the suppression state.

[danparizher]

I've implemented proper suppression state tracking by using SuppressionKind—the behavior works correctly with real files—it adds trailing newlines for non-suppressed regions and preserves formatting for suppressed regions.

What I couldn't figure out was that the test snapshot for Test Case 3 isn't showing the expected trailing newline, even though the actual formatter behavior is correct. Wondering if this is an issue with the fixture or intended behavior.

[MichaReiser]

This looks better, but I think is still incorrect for:

def test():
	# fmt: off
	...

This should only disable formatting inside the test function but not at the module level.

[MichaReiser]

You'll need to create different test files or you only test whatever fmt: off pattern comes last

[MichaReiser]

Unfortunatelly, it's more complicated than this:

a = b
    # fmt: off
b = 10

This is a module level fmt: off comment

But this isn't

if True:
	a = b
  # fmt: off

We should also support yapf: disable comments

ruff/crates/ruff_python_formatter/src/comments/mod.rs

Lines 169 to 171 in 32be31d

	pub(crate) fn is_suppression_off_comment(&self, text: &str) -> bool {
	SuppressionKind::is_suppression_off(self.text(text), self.line_position)
	}

We should also add a test for

# fmt: off

I think the right approach is to iterate over all statements and, for each statement, get its comments (using context.comments.leading_trailing(statement)). Then test if the last comment is a fmt: off comment

[MichaReiser]

We should avoid doing this check if the source ends with a \n.

[MichaReiser]

I suggest restructuring the entire code to

		body.format.with_options(SuiteKind::TopLevel).fmt(f)?;

		if source.ends_width('\n') || !has_unclosed_fmt_off(f.context()) {
			hard_line_break().fmt(f)?;
		}

[MichaReiser]

Is it intentional that this test case and module_level_trailing_off have a trailing newline?

[danparizher]

I refactored the modules and extracted a helper. Feel free to commit directly if there's still some missing logic; I know we've been going back and forth for a bit on this one!

[MichaReiser]

Hmm, I'm not sure what's happening. You have to do some debugging but I noticed that none of the tests are failing when I change this to (when I revert your changes)

Suggested change

	if source.ends_with(['\n', '\r']) || !has_unclosed_fmt_off(body, f.context()) {
	hard_line_break().fmt(f)?;
	}
	hard_line_break().fmt(f)?;

It seems we're still always inserting a trailing newline

[MichaReiser]

Thanks. I updated the fixture files to remove all trailing new lines to ensure they test the right thing. I also "reverted" your changes locally to verify that the tests start failing, which they don't. I'm not sure why. Can you take a look.

We should also add a test to verify that we preserve the correct number of blank lines for

test
# fmt: off
# many blank lines follow


<eof>

or

# empty file with many blank lines
# fmt: off


<eof>

Hmm... it seems that Github truncates the trailing blank lines. So I added a marker to prevent this

[danparizher]

I've spent some time debugging but wasn't able to make much headway. Not too sure what's going on either; I've tried multiple iterations of the same solution.

Might take another look later.

[MichaReiser]

I suspect that the statement formatting might insert a new line