mirror of
https://github.com/astral-sh/ruff.git
synced 2025-07-30 08:23:53 +00:00
Use OneIndexed in NotebookIndex (#7921)
## Summary This PR refactors the `NotebookIndex` struct to use `OneIndexed` to make the intent of the code clearer. ## Test Plan Update the existing test case and run `cargo test` to verify the change. - [x] Verify `--diff` output - [x] Verify the diagnostics output - [x] Verify `--show-source` output
This commit is contained in:
Dhruv Manilawala
GitHub
parent
c1fdb9c46d
commit
cd564c4200
6 changed files with 80 additions and 47 deletions
|
|
@ -177,18 +177,15 @@ impl Display for DisplayParseError<'_> {
|
|||
f,
|
||||
"cell {cell}{colon}",
|
||||
cell = jupyter_index
|
||||
.cell(source_location.row.get())
|
||||
.unwrap_or_default(),
|
||||
.cell(source_location.row)
|
||||
.unwrap_or(OneIndexed::MIN),
|
||||
colon = ":".cyan(),
|
||||
)?;
|
||||
|
||||
SourceLocation {
|
||||
row: OneIndexed::new(
|
||||
jupyter_index
|
||||
.cell_row(source_location.row.get())
|
||||
.unwrap_or(1) as usize,
|
||||
)
|
||||
.unwrap(),
|
||||
row: jupyter_index
|
||||
.cell_row(source_location.row)
|
||||
.unwrap_or(OneIndexed::MIN),
|
||||
column: source_location.column,
|
||||
}
|
||||
} else {
|
||||
|
|
|
|||
|
|
@ -125,18 +125,18 @@ impl Display for DisplayGroupedMessage<'_> {
|
|||
f,
|
||||
"cell {cell}{sep}",
|
||||
cell = jupyter_index
|
||||
.cell(start_location.row.get())
|
||||
.unwrap_or_default(),
|
||||
.cell(start_location.row)
|
||||
.unwrap_or(OneIndexed::MIN),
|
||||
sep = ":".cyan()
|
||||
)?;
|
||||
(
|
||||
jupyter_index
|
||||
.cell_row(start_location.row.get())
|
||||
.unwrap_or(1) as usize,
|
||||
start_location.column.get(),
|
||||
.cell_row(start_location.row)
|
||||
.unwrap_or(OneIndexed::MIN),
|
||||
start_location.column,
|
||||
)
|
||||
} else {
|
||||
(start_location.row.get(), start_location.column.get())
|
||||
(start_location.row, start_location.column)
|
||||
};
|
||||
|
||||
writeln!(
|
||||
|
|
|
|||
|
|
@ -87,18 +87,15 @@ impl Emitter for TextEmitter {
|
|||
writer,
|
||||
"cell {cell}{sep}",
|
||||
cell = notebook_index
|
||||
.cell(start_location.row.get())
|
||||
.unwrap_or_default(),
|
||||
.cell(start_location.row)
|
||||
.unwrap_or(OneIndexed::MIN),
|
||||
sep = ":".cyan(),
|
||||
)?;
|
||||
|
||||
SourceLocation {
|
||||
row: OneIndexed::new(
|
||||
notebook_index
|
||||
.cell_row(start_location.row.get())
|
||||
.unwrap_or(1) as usize,
|
||||
)
|
||||
.unwrap(),
|
||||
row: notebook_index
|
||||
.cell_row(start_location.row)
|
||||
.unwrap_or(OneIndexed::MIN),
|
||||
column: start_location.column,
|
||||
}
|
||||
} else {
|
||||
|
|
@ -203,9 +200,9 @@ impl Display for MessageCodeFrame<'_> {
|
|||
// If we're working with a Jupyter Notebook, skip the lines which are
|
||||
// outside of the cell containing the diagnostic.
|
||||
if let Some(index) = self.notebook_index {
|
||||
let content_start_cell = index.cell(content_start_index.get()).unwrap_or_default();
|
||||
let content_start_cell = index.cell(content_start_index).unwrap_or(OneIndexed::MIN);
|
||||
while start_index < content_start_index {
|
||||
if index.cell(start_index.get()).unwrap_or_default() == content_start_cell {
|
||||
if index.cell(start_index).unwrap_or(OneIndexed::MIN) == content_start_cell {
|
||||
break;
|
||||
}
|
||||
start_index = start_index.saturating_add(1);
|
||||
|
|
@ -228,9 +225,9 @@ impl Display for MessageCodeFrame<'_> {
|
|||
// If we're working with a Jupyter Notebook, skip the lines which are
|
||||
// outside of the cell containing the diagnostic.
|
||||
if let Some(index) = self.notebook_index {
|
||||
let content_end_cell = index.cell(content_end_index.get()).unwrap_or_default();
|
||||
let content_end_cell = index.cell(content_end_index).unwrap_or(OneIndexed::MIN);
|
||||
while end_index > content_end_index {
|
||||
if index.cell(end_index.get()).unwrap_or_default() == content_end_cell {
|
||||
if index.cell(end_index).unwrap_or(OneIndexed::MIN) == content_end_cell {
|
||||
break;
|
||||
}
|
||||
end_index = end_index.saturating_sub(1);
|
||||
|
|
@ -270,8 +267,9 @@ impl Display for MessageCodeFrame<'_> {
|
|||
|| start_index.get(),
|
||||
|notebook_index| {
|
||||
notebook_index
|
||||
.cell_row(start_index.get())
|
||||
.unwrap_or_default() as usize
|
||||
.cell_row(start_index)
|
||||
.unwrap_or(OneIndexed::MIN)
|
||||
.get()
|
||||
},
|
||||
),
|
||||
annotations: vec![SourceAnnotation {
|
||||
|
|
|
|||
|
|
@ -14,7 +14,7 @@ license = { workspace = true }
|
|||
|
||||
[dependencies]
|
||||
ruff_diagnostics = { path = "../ruff_diagnostics" }
|
||||
ruff_source_file = { path = "../ruff_source_file" }
|
||||
ruff_source_file = { path = "../ruff_source_file", features = ["serde"] }
|
||||
ruff_text_size = { path = "../ruff_text_size" }
|
||||
|
||||
anyhow = { workspace = true }
|
||||
|
|
|
|||
|
|
@ -1,5 +1,7 @@
|
|||
use serde::{Deserialize, Serialize};
|
||||
|
||||
use ruff_source_file::OneIndexed;
|
||||
|
||||
/// Jupyter Notebook indexing table
|
||||
///
|
||||
/// When we lint a jupyter notebook, we have to translate the row/column based on
|
||||
|
|
@ -7,20 +9,20 @@ use serde::{Deserialize, Serialize};
|
|||
#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
|
||||
pub struct NotebookIndex {
|
||||
/// Enter a row (1-based), get back the cell (1-based)
|
||||
pub(super) row_to_cell: Vec<u32>,
|
||||
pub(super) row_to_cell: Vec<OneIndexed>,
|
||||
/// Enter a row (1-based), get back the row in cell (1-based)
|
||||
pub(super) row_to_row_in_cell: Vec<u32>,
|
||||
pub(super) row_to_row_in_cell: Vec<OneIndexed>,
|
||||
}
|
||||
|
||||
impl NotebookIndex {
|
||||
/// Returns the cell number (1-based) for the given row (1-based).
|
||||
pub fn cell(&self, row: usize) -> Option<u32> {
|
||||
self.row_to_cell.get(row).copied()
|
||||
pub fn cell(&self, row: OneIndexed) -> Option<OneIndexed> {
|
||||
self.row_to_cell.get(row.to_zero_indexed()).copied()
|
||||
}
|
||||
|
||||
/// Returns the row number (1-based) in the cell (1-based) for the
|
||||
/// given row (1-based).
|
||||
pub fn cell_row(&self, row: usize) -> Option<u32> {
|
||||
self.row_to_row_in_cell.get(row).copied()
|
||||
pub fn cell_row(&self, row: OneIndexed) -> Option<OneIndexed> {
|
||||
self.row_to_row_in_cell.get(row.to_zero_indexed()).copied()
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@ use thiserror::Error;
|
|||
use uuid::Uuid;
|
||||
|
||||
use ruff_diagnostics::{SourceMap, SourceMarker};
|
||||
use ruff_source_file::{NewlineWithTrailingNewline, UniversalNewlineIterator};
|
||||
use ruff_source_file::{NewlineWithTrailingNewline, OneIndexed, UniversalNewlineIterator};
|
||||
use ruff_text_size::TextSize;
|
||||
|
||||
use crate::index::NotebookIndex;
|
||||
|
|
@ -179,7 +179,7 @@ impl Notebook {
|
|||
.iter()
|
||||
.enumerate()
|
||||
.filter(|(_, cell)| cell.is_valid_code_cell())
|
||||
.map(|(idx, _)| u32::try_from(idx).unwrap())
|
||||
.map(|(cell_index, _)| u32::try_from(cell_index).unwrap())
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
let mut contents = Vec::with_capacity(valid_code_cells.len());
|
||||
|
|
@ -321,16 +321,16 @@ impl Notebook {
|
|||
/// The index building is expensive as it needs to go through the content of
|
||||
/// every valid code cell.
|
||||
fn build_index(&self) -> NotebookIndex {
|
||||
let mut row_to_cell = vec![0];
|
||||
let mut row_to_row_in_cell = vec![0];
|
||||
let mut row_to_cell = Vec::new();
|
||||
let mut row_to_row_in_cell = Vec::new();
|
||||
|
||||
for &idx in &self.valid_code_cells {
|
||||
let line_count = match &self.raw.cells[idx as usize].source() {
|
||||
for &cell_index in &self.valid_code_cells {
|
||||
let line_count = match &self.raw.cells[cell_index as usize].source() {
|
||||
SourceValue::String(string) => {
|
||||
if string.is_empty() {
|
||||
1
|
||||
} else {
|
||||
u32::try_from(NewlineWithTrailingNewline::from(string).count()).unwrap()
|
||||
NewlineWithTrailingNewline::from(string).count()
|
||||
}
|
||||
}
|
||||
SourceValue::StringArray(string_array) => {
|
||||
|
|
@ -339,12 +339,14 @@ impl Notebook {
|
|||
} else {
|
||||
let trailing_newline =
|
||||
usize::from(string_array.last().is_some_and(|s| s.ends_with('\n')));
|
||||
u32::try_from(string_array.len() + trailing_newline).unwrap()
|
||||
string_array.len() + trailing_newline
|
||||
}
|
||||
}
|
||||
};
|
||||
row_to_cell.extend(iter::repeat(idx + 1).take(line_count as usize));
|
||||
row_to_row_in_cell.extend(1..=line_count);
|
||||
row_to_cell.extend(
|
||||
iter::repeat(OneIndexed::from_zero_indexed(cell_index as usize)).take(line_count),
|
||||
);
|
||||
row_to_row_in_cell.extend((0..line_count).map(OneIndexed::from_zero_indexed));
|
||||
}
|
||||
|
||||
NotebookIndex {
|
||||
|
|
@ -435,6 +437,8 @@ mod tests {
|
|||
use anyhow::Result;
|
||||
use test_case::test_case;
|
||||
|
||||
use ruff_source_file::OneIndexed;
|
||||
|
||||
use crate::{Cell, Notebook, NotebookError, NotebookIndex};
|
||||
|
||||
/// Construct a path to a Jupyter notebook in the `resources/test/fixtures/jupyter` directory.
|
||||
|
|
@ -514,8 +518,40 @@ print("after empty cells")
|
|||
assert_eq!(
|
||||
notebook.index(),
|
||||
&NotebookIndex {
|
||||
row_to_cell: vec![0, 1, 1, 1, 1, 1, 1, 3, 3, 3, 3, 3, 5, 7, 7, 8],
|
||||
row_to_row_in_cell: vec![0, 1, 2, 3, 4, 5, 6, 1, 2, 3, 4, 5, 1, 1, 2, 1],
|
||||
row_to_cell: vec![
|
||||
OneIndexed::from_zero_indexed(0),
|
||||
OneIndexed::from_zero_indexed(0),
|
||||
OneIndexed::from_zero_indexed(0),
|
||||
OneIndexed::from_zero_indexed(0),
|
||||
OneIndexed::from_zero_indexed(0),
|
||||
OneIndexed::from_zero_indexed(0),
|
||||
OneIndexed::from_zero_indexed(2),
|
||||
OneIndexed::from_zero_indexed(2),
|
||||
OneIndexed::from_zero_indexed(2),
|
||||
OneIndexed::from_zero_indexed(2),
|
||||
OneIndexed::from_zero_indexed(2),
|
||||
OneIndexed::from_zero_indexed(4),
|
||||
OneIndexed::from_zero_indexed(6),
|
||||
OneIndexed::from_zero_indexed(6),
|
||||
OneIndexed::from_zero_indexed(7)
|
||||
],
|
||||
row_to_row_in_cell: vec![
|
||||
OneIndexed::from_zero_indexed(0),
|
||||
OneIndexed::from_zero_indexed(1),
|
||||
OneIndexed::from_zero_indexed(2),
|
||||
OneIndexed::from_zero_indexed(3),
|
||||
OneIndexed::from_zero_indexed(4),
|
||||
OneIndexed::from_zero_indexed(5),
|
||||
OneIndexed::from_zero_indexed(0),
|
||||
OneIndexed::from_zero_indexed(1),
|
||||
OneIndexed::from_zero_indexed(2),
|
||||
OneIndexed::from_zero_indexed(3),
|
||||
OneIndexed::from_zero_indexed(4),
|
||||
OneIndexed::from_zero_indexed(0),
|
||||
OneIndexed::from_zero_indexed(0),
|
||||
OneIndexed::from_zero_indexed(1),
|
||||
OneIndexed::from_zero_indexed(0)
|
||||
],
|
||||
}
|
||||
);
|
||||
assert_eq!(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue