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

Add test and basic implementation for formatter preview mode #8044

Merged
merged 6 commits into from
Oct 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 8 additions & 5 deletions crates/ruff_dev/src/format_dev.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use ruff_formatter::{FormatError, LineWidth, PrintError};
use ruff_linter::logging::LogLevel;
use ruff_linter::settings::types::{FilePattern, FilePatternSet};
use ruff_python_formatter::{
format_module_source, FormatModuleError, MagicTrailingComma, PyFormatOptions,
format_module_source, FormatModuleError, MagicTrailingComma, PreviewMode, PyFormatOptions,
};
use ruff_workspace::resolver::{python_files_in_path, PyprojectConfig, ResolvedFile, Resolver};

Expand Down Expand Up @@ -871,17 +871,15 @@ struct BlackOptions {
line_length: NonZeroU16,
#[serde(alias = "skip-magic-trailing-comma")]
skip_magic_trailing_comma: bool,
#[allow(unused)]
#[serde(alias = "force-exclude")]
force_exclude: Option<String>,
preview: bool,
}

impl Default for BlackOptions {
fn default() -> Self {
Self {
line_length: NonZeroU16::new(88).unwrap(),
skip_magic_trailing_comma: false,
force_exclude: None,
preview: false,
}
}
}
Expand Down Expand Up @@ -929,6 +927,11 @@ impl BlackOptions {
} else {
MagicTrailingComma::Respect
})
.with_preview(if self.preview {
PreviewMode::Enabled
} else {
PreviewMode::Disabled
})
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,39 @@
# Below is black stable style
# In preview style, black always breaks the right side first
"""
Black's `Preview.module_docstring_newlines`
"""
first_stmt_after_module_level_docstring = 1

if True:

class CachedRepository:
# Black's `Preview.dummy_implementations`
def get_release_info(self): ...


def raw_docstring():

r"""Black's `Preview.accept_raw_docstrings`
a
b
"""
pass


def reference_docstring_newlines():

"""A regular docstring for comparison
a
b
"""
pass


class RemoveNewlineBeforeClassDocstring:

"""Black's `Preview.no_blank_line_before_class_docstring`"""


def f():
"""Black's `Preview.prefer_splitting_right_hand_side_of_assignments`"""
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa[
bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
] = cccccccc.ccccccccccccc.cccccccc
Expand Down
11 changes: 9 additions & 2 deletions crates/ruff_python_formatter/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use ruff_python_parser::{parse_ok_tokens, AsMode};
use ruff_text_size::Ranged;

use crate::comments::collect_comments;
use crate::{format_module_ast, PyFormatOptions};
use crate::{format_module_ast, PreviewMode, PyFormatOptions};

#[derive(ValueEnum, Clone, Debug)]
pub enum Emit {
Expand All @@ -24,6 +24,7 @@ pub enum Emit {

#[derive(Parser)]
#[command(author, version, about, long_about = None)]
#[allow(clippy::struct_excessive_bools)] // It's only the dev cli anyways
pub struct Cli {
/// Python files to format. If there are none, stdin will be used. `-` as stdin is not supported
pub files: Vec<PathBuf>,
Expand All @@ -34,6 +35,8 @@ pub struct Cli {
#[clap(long)]
pub check: bool,
#[clap(long)]
pub preview: bool,
#[clap(long)]
pub print_ir: bool,
#[clap(long)]
pub print_comments: bool,
Expand All @@ -48,7 +51,11 @@ pub fn format_and_debug_print(source: &str, cli: &Cli, source_path: &Path) -> Re
let module = parse_ok_tokens(tokens, source, source_type.as_mode(), "<filename>")
.context("Syntax error in input")?;

let options = PyFormatOptions::from_extension(source_path);
let options = PyFormatOptions::from_extension(source_path).with_preview(if cli.preview {
PreviewMode::Enabled
} else {
PreviewMode::Disabled
});

let source_code = SourceCode::new(source);
let formatted = format_module_ast(&module, &comment_ranges, source, options)
Expand Down
4 changes: 3 additions & 1 deletion crates/ruff_python_formatter/src/statement/clause.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,9 @@ pub(crate) fn clause_body<'a>(

impl Format<PyFormatContext<'_>> for FormatClauseBody<'_> {
fn fmt(&self, f: &mut Formatter<PyFormatContext<'_>>) -> FormatResult<()> {
if f.options().source_type().is_stub()
// In stable, stubs are only collapsed in stub files, in preview this is consistently
// applied everywhere
if (f.options().source_type().is_stub() || f.options().preview().is_enabled())
&& contains_only_an_ellipsis(self.body, f.context().comments())
&& self.trailing_comments.is_empty()
{
Expand Down
42 changes: 33 additions & 9 deletions crates/ruff_python_formatter/tests/fixtures.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use ruff_formatter::FormatOptions;
use ruff_python_formatter::{format_module_source, PyFormatOptions};
use ruff_python_formatter::{format_module_source, PreviewMode, PyFormatOptions};
use similar::TextDiff;
use std::fmt::{Formatter, Write};
use std::io::BufReader;
Expand Down Expand Up @@ -142,16 +142,40 @@ fn format() {
} else {
let printed =
format_module_source(&content, options.clone()).expect("Formatting to succeed");
let formatted_code = printed.as_code();
let formatted = printed.as_code();

ensure_stability_when_formatting_twice(formatted_code, options, input_path);
ensure_stability_when_formatting_twice(formatted, options.clone(), input_path);

writeln!(
snapshot,
"## Output\n{}",
CodeFrame::new("py", &formatted_code)
)
.unwrap();
// We want to capture the differences in the preview style in our fixtures
let options_preview = options.with_preview(PreviewMode::Enabled);
Copy link
Member

Choose a reason for hiding this comment

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

Should we skip this if options.preview == Enabled when using a test specific options file?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the no-options-file branch; i think it's better to not mix them to avoid combinatorial explosion

let printed_preview = format_module_source(&content, options_preview.clone())
.expect("Formatting to succeed");
let formatted_preview = printed_preview.as_code();

ensure_stability_when_formatting_twice(
formatted_preview,
options_preview.clone(),
input_path,
);

if formatted == formatted_preview {
writeln!(snapshot, "## Output\n{}", CodeFrame::new("py", &formatted)).unwrap();
} else {
// Having both snapshots makes it hard to see the difference, so we're keeping only
// diff.
writeln!(
snapshot,
"## Output\n{}\n## Preview changes\n{}",
CodeFrame::new("py", &formatted),
CodeFrame::new(
"diff",
TextDiff::from_lines(formatted, formatted_preview)
.unified_diff()
.header("Stable", "Preview")
)
)
.unwrap();
}
}

insta::with_settings!({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,4 +93,21 @@ def test3 ():
```


## Preview changes
```diff
--- Stable
+++ Preview
@@ -21,8 +21,7 @@


# formatted
-def test2():
- ...
+def test2(): ...


a = 10
```



Original file line number Diff line number Diff line change
Expand Up @@ -549,4 +549,27 @@ if True:
```


## Preview changes
```diff
--- Stable
+++ Preview
@@ -245,13 +245,11 @@
class Path:
if sys.version_info >= (3, 11):

- def joinpath(self):
- ...
+ def joinpath(self): ...

# The .open method comes from pathlib.pyi and should be kept in sync.
@overload
- def open(self):
- ...
+ def open(self): ...


def fakehttp():
```



Original file line number Diff line number Diff line change
@@ -1,13 +1,45 @@
---
source: crates/ruff_python_formatter/tests/fixtures.rs
input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/statement/assign_breaking.py
input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/preview.py
Copy link
Member

Choose a reason for hiding this comment

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

Should we instead run every test under preview and non-preview?

Copy link
Member

Choose a reason for hiding this comment

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

If the cost is not too high that seems ideal

Copy link
Member Author

Choose a reason for hiding this comment

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

hyperfine "cargo test -p ruff_python_formatter" goes from 500ms to 580ms on my machine, acceptable for me.

Having both versions makes it hard to see what actually changed given our large snapshots, so i opted for instead adding the diff as separate code block if there is one. I also changed all non-function/class tests to use pass instead as real code generally does, which i split out to #8049 to keep the diff readable.

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 would like to keep preview.py as-is though, it's a useful tool for tracking progress and comparing to black (i can diff both snapshots with what the black playground does)

---
## Input
```py
# Below is black stable style
# In preview style, black always breaks the right side first
"""
Black's `Preview.module_docstring_newlines`
"""
first_stmt_after_module_level_docstring = 1

if True:

class CachedRepository:
# Black's `Preview.dummy_implementations`
def get_release_info(self): ...


def raw_docstring():

r"""Black's `Preview.accept_raw_docstrings`
a
b
"""
pass


def reference_docstring_newlines():

"""A regular docstring for comparison
a
b
"""
pass


class RemoveNewlineBeforeClassDocstring:

"""Black's `Preview.no_blank_line_before_class_docstring`"""


def f():
"""Black's `Preview.prefer_splitting_right_hand_side_of_assignments`"""
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa[
bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
] = cccccccc.ccccccccccccc.cccccccc
Expand Down Expand Up @@ -52,10 +84,41 @@ preview = Disabled
```

```py
# Below is black stable style
# In preview style, black always breaks the right side first
"""
Black's `Preview.module_docstring_newlines`
"""
first_stmt_after_module_level_docstring = 1


class CachedRepository:
# Black's `Preview.dummy_implementations`
def get_release_info(self):
...


def raw_docstring():
r"""Black's `Preview.accept_raw_docstrings`
a
b
"""
pass

if True:

def reference_docstring_newlines():
"""A regular docstring for comparison
a
b
"""
pass


class RemoveNewlineBeforeClassDocstring:

"""Black's `Preview.no_blank_line_before_class_docstring`"""


def f():
"""Black's `Preview.prefer_splitting_right_hand_side_of_assignments`"""
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa[
bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
] = cccccccc.ccccccccccccc.cccccccc
Expand Down Expand Up @@ -100,10 +163,40 @@ preview = Enabled
```

```py
# Below is black stable style
# In preview style, black always breaks the right side first
"""
Black's `Preview.module_docstring_newlines`
"""
first_stmt_after_module_level_docstring = 1


class CachedRepository:
# Black's `Preview.dummy_implementations`
def get_release_info(self): ...


def raw_docstring():
r"""Black's `Preview.accept_raw_docstrings`
a
b
"""
pass


def reference_docstring_newlines():
"""A regular docstring for comparison
a
b
"""
pass


class RemoveNewlineBeforeClassDocstring:

"""Black's `Preview.no_blank_line_before_class_docstring`"""


if True:
def f():
"""Black's `Preview.prefer_splitting_right_hand_side_of_assignments`"""
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa[
bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
] = cccccccc.ccccccccccccc.cccccccc
Expand Down