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

Use ruff line-length in format_dev #6870

Merged
merged 2 commits into from
Sep 5, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 36 additions & 10 deletions crates/ruff_dev/src/format_dev.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ use imara_diff::{diff, Algorithm};
use indicatif::ProgressStyle;
#[cfg_attr(feature = "singlethreaded", allow(unused_imports))]
use rayon::iter::{IntoParallelIterator, ParallelIterator};
use ruff::line_width::LineLength;
use serde::Deserialize;
use similar::{ChangeTag, TextDiff};
use tempfile::NamedTempFile;
Expand All @@ -36,10 +37,17 @@ use ruff_formatter::{FormatError, LineWidth, PrintError};
use ruff_python_formatter::{
format_module, FormatModuleError, MagicTrailingComma, PyFormatOptions,
};
use ruff_workspace::resolver::python_files_in_path;
use ruff_workspace::resolver::{python_files_in_path, PyprojectConfig, Resolver};

/// Find files that ruff would check so we can format them. Adapted from `ruff_cli`.
fn ruff_check_paths(dirs: &[PathBuf]) -> anyhow::Result<Vec<Result<DirEntry, ignore::Error>>> {
#[allow(clippy::type_complexity)]
fn ruff_check_paths(
dirs: &[PathBuf],
) -> anyhow::Result<(
Vec<Result<DirEntry, ignore::Error>>,
Resolver,
PyprojectConfig,
)> {
let args_matches = FormatCommand::command()
.no_binary_name(true)
.get_matches_from(dirs);
Expand All @@ -57,11 +65,11 @@ fn ruff_check_paths(dirs: &[PathBuf]) -> anyhow::Result<Vec<Result<DirEntry, ign
FilePattern::Builtin("*.pyi"),
])
.unwrap();
let (paths, _resolver) = python_files_in_path(&cli.files, &pyproject_config, &overrides)?;
let (paths, resolver) = python_files_in_path(&cli.files, &pyproject_config, &overrides)?;
if paths.is_empty() {
bail!("no python files in {:?}", dirs)
}
Ok(paths)
Ok((paths, resolver, pyproject_config))
}

/// Collects statistics over the formatted files to compute the Jaccard index or the similarity
Expand Down Expand Up @@ -448,7 +456,7 @@ fn format_dev_project(

// Find files to check (or in this case, format twice). Adapted from ruff_cli
// First argument is ignored
let paths = ruff_check_paths(files)?;
let (paths, resolver, pyproject_config) = ruff_check_paths(files)?;

let results = {
let pb_span =
Expand All @@ -461,7 +469,14 @@ fn format_dev_project(
#[cfg(feature = "singlethreaded")]
let iter = { paths.into_iter() };
iter.map(|dir_entry| {
let result = format_dir_entry(dir_entry, stability_check, write, &black_options);
let result = format_dir_entry(
dir_entry,
stability_check,
write,
&black_options,
&resolver,
&pyproject_config,
);
pb_span.pb_inc(1);
result
})
Expand Down Expand Up @@ -517,6 +532,8 @@ fn format_dir_entry(
stability_check: bool,
write: bool,
options: &BlackOptions,
resolver: &Resolver,
pyproject_config: &PyprojectConfig,
) -> anyhow::Result<(Result<Statistics, CheckFileError>, PathBuf), Error> {
let dir_entry = match dir_entry.context("Iterating the files in the repository failed") {
Ok(dir_entry) => dir_entry,
Expand All @@ -528,10 +545,19 @@ fn format_dir_entry(
return Ok((Ok(Statistics::default()), file));
}

let file = dir_entry.path().to_path_buf();
let options = options.to_py_format_options(&file);
let path = dir_entry.path().to_path_buf();
let mut options = options.to_py_format_options(&path);

let settings = resolver.resolve(&path, pyproject_config);
// That's a bad way of doing this but it's not worth doing something better for format_dev
if settings.line_length != LineLength::default() {
Copy link
Member

Choose a reason for hiding this comment

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

Is this because you're trying to see if it's not the default? Why can't we just use settings.line_length? Because it will be wrong for projects in the formatter CI that use Black but not Ruff?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, this only exists to have a (dummy) usage for the line length without breaking the formatter ecosystem checks

Copy link
Member

Choose a reason for hiding this comment

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

Should Settings.line_length be an Option<LineLength> if that's something we want to distinguish?

let line_width = LineWidth::try_from(settings.line_length.value())
.expect("Configured line length is too large for the formatter");
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 still don't like this divergence

Copy link
Member

Choose a reason for hiding this comment

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

You can do LineWidth::from(NonZeroU16::from(line_length)))

options = options.with_line_width(line_width);
}

// Handle panics (mostly in `debug_assert!`)
let result = match catch_unwind(|| format_dev_file(&file, stability_check, write, options)) {
let result = match catch_unwind(|| format_dev_file(&path, stability_check, write, options)) {
Ok(result) => result,
Err(panic) => {
if let Some(message) = panic.downcast_ref::<String>() {
Expand All @@ -550,7 +576,7 @@ fn format_dir_entry(
}
}
};
Ok((result, file))
Ok((result, path))
}

/// A compact diff that only shows a header and changes, but nothing unchanged. This makes viewing
Expand Down
5 changes: 1 addition & 4 deletions crates/ruff_python_formatter/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -273,10 +273,7 @@ for converter in connection.ops.get_db_converters(
struct FormatString<'a>(&'a str);

impl Format<SimpleFormatContext> for FormatString<'_> {
fn fmt(
&self,
f: &mut ruff_formatter::formatter::Formatter<SimpleFormatContext>,
) -> FormatResult<()> {
fn fmt(&self, f: &mut Formatter<SimpleFormatContext>) -> FormatResult<()> {
let format_str = format_with(|f| {
write!(f, [token("\"")])?;

Expand Down
Loading