Skip to content

Commit

Permalink
Use OneIndexed in NotebookIndex (#7921)
Browse files Browse the repository at this point in the history 
## 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
  • Loading branch information
@dhruvmanila
dhruvmanila authored Oct 13, 2023
1 parent c1fdb9c commit cd564c4
Show file tree
Hide file tree
Showing 6 changed files with 80 additions and 47 deletions.
13 changes: 5 additions & 8 deletions crates/ruff_linter/src/logging.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
12 changes: 6 additions & 6 deletions crates/ruff_linter/src/message/grouped.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down
26 changes: 12 additions & 14 deletions crates/ruff_linter/src/message/text.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_notebook/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
14 changes: 8 additions & 6 deletions crates/ruff_notebook/src/index.rs
View file
Original file line number Diff line number Diff line change
@@ -1,26 +1,28 @@
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
/// [`ruff_text_size::TextSize`] to jupyter notebook cell/row/column.
#[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()
}
}
60 changes: 48 additions & 12 deletions crates/ruff_notebook/src/notebook.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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) => {
Expand All @@ -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 {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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!(
Expand Down

0 comments on commit cd564c4

Please sign in to comment.