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.

Sign up for GitHub

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

Jump to bottom

Compute NotebookIndex for Diagnostics on stdin #7663

Merged
merged 4 commits into from
Sep 29, 2023
Merged

Compute NotebookIndex for Diagnostics on stdin #7663

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
404b53e
Use 1-based cell indices consistently for Notebooks
dhruvmanila Sep 26, 2023
a91285c
Compute `NotebookIndex` to `Diagnostics` on stdin
dhruvmanila Sep 26, 2023
dcfe1d0
Merge branch 'main' into dhruv/notebook-index-stdin
charliermarsh Sep 29, 2023
089c7fa
Remove feedback
charliermarsh Sep 29, 2023
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
25 changes: 16 additions & 9 deletions crates/ruff_cli/src/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use std::ops::AddAssign;
use std::os::unix::fs::PermissionsExt;
use std::path::Path;

use anyhow::{anyhow, Context, Result};
use anyhow::{Context, Result};
use colored::Colorize;
use filetime::FileTime;
use log::{debug, error, warn};
Expand Down Expand Up @@ -254,10 +254,9 @@ pub(crate) fn lint_path(
// mutated it.
let src_notebook = source_kind.as_ipy_notebook().unwrap();
let mut stdout = io::stdout().lock();
for ((idx, src_cell), dest_cell) in src_notebook
.cells()
.iter()
.enumerate()
// Cell indices are 1-based.
for ((idx, src_cell), dest_cell) in (1u32..)
.zip(src_notebook.cells().iter())
.zip(dest_notebook.cells().iter())
{
let (Cell::Code(src_code_cell), Cell::Code(dest_code_cell)) =
Expand Down Expand Up @@ -344,9 +343,7 @@ pub(crate) fn lint_path(

let notebook_indexes = if let SourceKind::IpyNotebook(notebook) = source_kind {
FxHashMap::from_iter([(
path.to_str()
.ok_or_else(|| anyhow!("Unable to parse filename: {:?}", path))?
.to_string(),
path.to_string_lossy().to_string(),
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 change is to keep the value consistent. We use .to_string_lossy everywhere.

// Index needs to be computed always to store in cache.
notebook.index().clone(),
)])
Expand Down Expand Up @@ -475,14 +472,24 @@ pub(crate) fn lint_stdin(
);
}

let notebook_indexes = if let SourceKind::IpyNotebook(notebook) = source_kind {
FxHashMap::from_iter([(
path.map_or_else(|| "-".into(), |path| path.to_string_lossy().to_string()),
// Index needs to be computed always to store in cache.
Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that we don't perform any caching when using stdin.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is copied over from lint_path, but I don't fully understand it in the context of lint_path. Don't we need to compute the index to report diagnostics, not just store in the cache?

Copy link
Member

Choose a reason for hiding this comment

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

I removed these comments as I found them sorta unhelpful.

Copy link
Member

Choose a reason for hiding this comment

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

(I think this comment made sense in the PR in which it was introduced because it was a change, but it's less helpful in isolation.)

notebook.index().clone(),
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Add an into_index() method to avoid cloning the notebook

Copy link
Member

Choose a reason for hiding this comment

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

Added.

)])
} else {
FxHashMap::default()
};

Ok(Diagnostics {
messages,
fixed: FxHashMap::from_iter([(
fs::relativize_path(path.unwrap_or_else(|| Path::new("-"))),
fixed,
)]),
imports,
notebook_indexes: FxHashMap::default(),
notebook_indexes,
})
}

Expand Down