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

Avoid loading files for cached format results #8134

Merged
merged 1 commit into from Oct 23, 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
2 changes: 0 additions & 2 deletions crates/ruff_cli/src/cache.rs
Expand Up @@ -571,7 +571,6 @@ mod tests {
use ruff_cache::CACHE_DIR_NAME;
use ruff_linter::settings::flags;
use ruff_linter::settings::types::UnsafeFixes;
use ruff_linter::source_kind::SourceKind;
use ruff_python_ast::PySourceType;
use ruff_workspace::Settings;

Expand Down Expand Up @@ -1061,7 +1060,6 @@ mod tests {
format_path(
&file_path,
&self.settings.formatter,
&SourceKind::Python(std::fs::read_to_string(&file_path).unwrap()),
PySourceType::Python,
FormatMode::Write,
Some(cache),
Expand Down
54 changes: 31 additions & 23 deletions crates/ruff_cli/src/commands/format.rs
Expand Up @@ -124,18 +124,6 @@ pub(crate) fn format(
return None;
}

// Extract the sources from the file.
let unformatted = match SourceKind::from_path(path, source_type) {
Ok(Some(source_kind)) => source_kind,
Ok(None) => return None,
Err(err) => {
return Some(Err(FormatCommandError::Read(
Some(path.to_path_buf()),
err,
)));
}
};

let package = path
.parent()
.and_then(|parent| package_roots.get(parent).copied())
Expand All @@ -148,15 +136,13 @@ pub(crate) fn format(
format_path(
path,
&resolved_settings.formatter,
&unformatted,
source_type,
mode,
cache,
)
}) {
Ok(inner) => inner.map(|result| FormatPathResult {
path: resolved_file.path().to_path_buf(),
unformatted,
result,
}),
Err(error) => Err(FormatCommandError::Panic(
Expand Down Expand Up @@ -253,7 +239,6 @@ pub(crate) fn format(
pub(crate) fn format_path(
path: &Path,
settings: &FormatterSettings,
unformatted: &SourceKind,
source_type: PySourceType,
mode: FormatMode,
cache: Option<&Cache>,
Expand All @@ -270,8 +255,18 @@ pub(crate) fn format_path(
}
}

// Extract the sources from the file.
let unformatted = match SourceKind::from_path(path, source_type) {
Ok(Some(source_kind)) => source_kind,
// Non Python Jupyter notebook
Ok(None) => return Ok(FormatResult::Skipped),
Err(err) => {
return Err(FormatCommandError::Read(Some(path.to_path_buf()), err));
}
};

// Format the source.
let format_result = match format_source(unformatted, source_type, Some(path), settings)? {
let format_result = match format_source(&unformatted, source_type, Some(path), settings)? {
FormattedSource::Formatted(formatted) => match mode {
FormatMode::Write => {
let mut writer = File::create(path).map_err(|err| {
Expand All @@ -293,7 +288,10 @@ pub(crate) fn format_path(
FormatResult::Formatted
}
FormatMode::Check => FormatResult::Formatted,
FormatMode::Diff => FormatResult::Diff(formatted),
FormatMode::Diff => FormatResult::Diff {
unformatted,
formatted,
},
},
FormattedSource::Unchanged => {
if let Some(cache) = cache {
Expand Down Expand Up @@ -425,16 +423,21 @@ pub(crate) enum FormatResult {
/// The file was formatted.
Formatted,
/// The file was formatted, [`SourceKind`] contains the formatted code
Diff(SourceKind),
Diff {
unformatted: SourceKind,
formatted: SourceKind,
},
/// The file was unchanged, as the formatted contents matched the existing contents.
Unchanged,

/// Skipped formatting because its an unformatted file format
Skipped,
Comment on lines -428 to +434
Copy link
Member

Choose a reason for hiding this comment

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

This was easier than i expected

}

/// The coupling of a [`FormatResult`] with the path of the file that was analyzed.
#[derive(Debug)]
struct FormatPathResult {
path: PathBuf,
unformatted: SourceKind,
result: FormatResult,
}

Expand All @@ -456,15 +459,19 @@ impl<'a> FormatResults<'a> {
fn any_formatted(&self) -> bool {
self.results.iter().any(|result| match result.result {
FormatResult::Formatted | FormatResult::Diff { .. } => true,
FormatResult::Unchanged => false,
FormatResult::Unchanged | FormatResult::Skipped => false,
})
}

fn write_diff(&self, f: &mut impl Write) -> io::Result<()> {
for result in self.results {
if let FormatResult::Diff(formatted) = &result.result {
if let FormatResult::Diff {
unformatted,
formatted,
} = &result.result
{
let text_diff =
TextDiff::from_lines(result.unformatted.source_code(), formatted.source_code());
TextDiff::from_lines(unformatted.source_code(), formatted.source_code());
let mut unified_diff = text_diff.unified_diff();
unified_diff.header(
&fs::relativize_path(&result.path),
Expand Down Expand Up @@ -495,9 +502,10 @@ impl<'a> FormatResults<'a> {
changed += 1;
}
FormatResult::Unchanged => unchanged += 1,
FormatResult::Diff(_) => {
FormatResult::Diff { .. } => {
changed += 1;
}
FormatResult::Skipped => {}
}
}

Expand Down