From 913a357173d618d19b8d64aef94526b345f74227 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Wed, 11 Oct 2023 22:56:41 +0530 Subject: [PATCH] Use `OneIndexed` for `NotebookIndex` --- crates/ruff_linter/src/logging.rs | 13 ++--- crates/ruff_linter/src/message/grouped.rs | 12 ++--- crates/ruff_linter/src/message/text.rs | 26 +++++----- crates/ruff_notebook/Cargo.toml | 2 +- crates/ruff_notebook/src/index.rs | 14 +++--- crates/ruff_notebook/src/notebook.rs | 60 ++++++++++++++++++----- 6 files changed, 80 insertions(+), 47 deletions(-) diff --git a/crates/ruff_linter/src/logging.rs b/crates/ruff_linter/src/logging.rs index 6851680cfb0a73..d65acd9ad58f32 100644 --- a/crates/ruff_linter/src/logging.rs +++ b/crates/ruff_linter/src/logging.rs @@ -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 { diff --git a/crates/ruff_linter/src/message/grouped.rs b/crates/ruff_linter/src/message/grouped.rs index df01b8a9a41079..0445c1746193bf 100644 --- a/crates/ruff_linter/src/message/grouped.rs +++ b/crates/ruff_linter/src/message/grouped.rs @@ -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!( diff --git a/crates/ruff_linter/src/message/text.rs b/crates/ruff_linter/src/message/text.rs index 867f92bc031deb..67bf97fe999e97 100644 --- a/crates/ruff_linter/src/message/text.rs +++ b/crates/ruff_linter/src/message/text.rs @@ -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 { diff --git a/crates/ruff_notebook/Cargo.toml b/crates/ruff_notebook/Cargo.toml index 7b3dfde8f6a6b5..0796757ca36e5b 100644 --- a/crates/ruff_notebook/Cargo.toml +++ b/crates/ruff_notebook/Cargo.toml @@ -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 } diff --git a/crates/ruff_notebook/src/index.rs b/crates/ruff_notebook/src/index.rs index 23259468ee56f4..c6207ef27e69a5 100644 --- a/crates/ruff_notebook/src/index.rs +++ b/crates/ruff_notebook/src/index.rs @@ -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, + pub(super) row_to_cell: Vec, /// Enter a row (1-based), get back the row in cell (1-based) - pub(super) row_to_row_in_cell: Vec, + pub(super) row_to_row_in_cell: Vec, } impl NotebookIndex { /// Returns the cell number (1-based) for the given row (1-based). - pub fn cell(&self, row: usize) -> Option { - self.row_to_cell.get(row).copied() + pub fn cell(&self, row: OneIndexed) -> Option { + 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 { - self.row_to_row_in_cell.get(row).copied() + pub fn cell_row(&self, row: OneIndexed) -> Option { + self.row_to_row_in_cell.get(row.to_zero_indexed()).copied() } } diff --git a/crates/ruff_notebook/src/notebook.rs b/crates/ruff_notebook/src/notebook.rs index f1e3e73bffeb99..34562473c4f52a 100644 --- a/crates/ruff_notebook/src/notebook.rs +++ b/crates/ruff_notebook/src/notebook.rs @@ -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::>(); 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!(