Skip to content

Commit 6e9ff44

Browse files
authored
Insert the cells from the start position (#15398)
## Summary The cause of this bug is from #12575 which was itself a bug fix but the fix wasn't completely correct. fixes: #14768 fixes: astral-sh/ruff-vscode#644 ## Test Plan Consider the following three cells: 1. ```python class Foo: def __init__(self): self.x = 1 def __str__(self): return f"Foo({self.x})" ``` 2. ```python def hello(): print("hello world") ``` 3. ```python y = 1 ``` The test case is moving cell 2 to the top i.e., cell 2 goes to position 1 and cell 1 goes to position 2. Before this fix, it can be seen that the cells were pushed at the end of the vector: ``` 12.643269917s INFO ruff:main ruff_server::edit::notebook: Before update: [ NotebookCell { document: TextDocument { contents: "class Foo:\n def __init__(self):\n self.x = 1\n\n def __str__(self):\n return f\"Foo({self.x})\"", }, }, NotebookCell { document: TextDocument { contents: "def hello():\n print(\"hello world\")", }, }, NotebookCell { document: TextDocument { contents: "y = 1", }, }, ] 12.643777667s INFO ruff:main ruff_server::edit::notebook: After update: [ NotebookCell { document: TextDocument { contents: "y = 1", }, }, NotebookCell { document: TextDocument { contents: "class Foo:\n def __init__(self):\n self.x = 1\n\n def __str__(self):\n return f\"Foo({self.x})\"", }, }, NotebookCell { document: TextDocument { contents: "def hello():\n print(\"hello world\")", }, }, ] ``` After the fix in this PR, it can be seen that the cells are being pushed at the correct `start` index: ``` 6.520570917s INFO ruff:main ruff_server::edit::notebook: Before update: [ NotebookCell { document: TextDocument { contents: "class Foo:\n def __init__(self):\n self.x = 1\n\n def __str__(self):\n return f\"Foo({self.x})\"", }, }, NotebookCell { document: TextDocument { contents: "def hello():\n print(\"hello world\")", }, }, NotebookCell { document: TextDocument { contents: "y = 1", }, }, ] 6.521084792s INFO ruff:main ruff_server::edit::notebook: After update: [ NotebookCell { document: TextDocument { contents: "def hello():\n print(\"hello world\")", }, }, NotebookCell { document: TextDocument { contents: "class Foo:\n def __init__(self):\n self.x = 1\n\n def __str__(self):\n return f\"Foo({self.x})\"", }, }, NotebookCell { document: TextDocument { contents: "y = 1", }, }, ] ```
1 parent f2c3ddc commit 6e9ff44

File tree

1 file changed

+121
-11
lines changed

1 file changed

+121
-11
lines changed

crates/ruff_server/src/edit/notebook.rs

+121-11
Original file line numberDiff line numberDiff line change
@@ -136,17 +136,15 @@ impl NotebookDocument {
136136
// provide the actual contents of the cells, so we'll initialize them with empty
137137
// contents.
138138
for cell in structure.array.cells.into_iter().flatten().rev() {
139-
if let Some(text_document) = deleted_cells.remove(&cell.document) {
140-
let version = text_document.version();
141-
self.cells.push(NotebookCell::new(
142-
cell,
143-
text_document.into_contents(),
144-
version,
145-
));
146-
} else {
147-
self.cells
148-
.insert(start, NotebookCell::new(cell, String::new(), 0));
149-
}
139+
let (content, version) =
140+
if let Some(text_document) = deleted_cells.remove(&cell.document) {
141+
let version = text_document.version();
142+
(text_document.into_contents(), version)
143+
} else {
144+
(String::new(), 0)
145+
};
146+
self.cells
147+
.insert(start, NotebookCell::new(cell, content, version));
150148
}
151149

152150
// Third, register the new cells in the index and update existing ones that came
@@ -243,3 +241,115 @@ impl NotebookCell {
243241
}
244242
}
245243
}
244+
245+
#[cfg(test)]
246+
mod tests {
247+
use super::NotebookDocument;
248+
249+
enum TestCellContent {
250+
#[allow(dead_code)]
251+
Markup(String),
252+
Code(String),
253+
}
254+
255+
fn create_test_url(index: usize) -> lsp_types::Url {
256+
lsp_types::Url::parse(&format!("cell:/test.ipynb#{index}")).unwrap()
257+
}
258+
259+
fn create_test_notebook(test_cells: Vec<TestCellContent>) -> NotebookDocument {
260+
let mut cells = Vec::with_capacity(test_cells.len());
261+
let mut cell_documents = Vec::with_capacity(test_cells.len());
262+
263+
for (index, test_cell) in test_cells.into_iter().enumerate() {
264+
let url = create_test_url(index);
265+
match test_cell {
266+
TestCellContent::Markup(content) => {
267+
cells.push(lsp_types::NotebookCell {
268+
kind: lsp_types::NotebookCellKind::Markup,
269+
document: url.clone(),
270+
metadata: None,
271+
execution_summary: None,
272+
});
273+
cell_documents.push(lsp_types::TextDocumentItem {
274+
uri: url,
275+
language_id: "markdown".to_owned(),
276+
version: 0,
277+
text: content,
278+
});
279+
}
280+
TestCellContent::Code(content) => {
281+
cells.push(lsp_types::NotebookCell {
282+
kind: lsp_types::NotebookCellKind::Code,
283+
document: url.clone(),
284+
metadata: None,
285+
execution_summary: None,
286+
});
287+
cell_documents.push(lsp_types::TextDocumentItem {
288+
uri: url,
289+
language_id: "python".to_owned(),
290+
version: 0,
291+
text: content,
292+
});
293+
}
294+
}
295+
}
296+
297+
NotebookDocument::new(0, cells, serde_json::Map::default(), cell_documents).unwrap()
298+
}
299+
300+
/// This test case checks that for a notebook with three code cells, when the client sends a
301+
/// change request to swap the first two cells, the notebook document is updated correctly.
302+
///
303+
/// The swap operation as a change request is represented as deleting the first two cells and
304+
/// adding them back in the reverse order.
305+
#[test]
306+
fn swap_cells() {
307+
let mut notebook = create_test_notebook(vec![
308+
TestCellContent::Code("cell = 0".to_owned()),
309+
TestCellContent::Code("cell = 1".to_owned()),
310+
TestCellContent::Code("cell = 2".to_owned()),
311+
]);
312+
313+
notebook
314+
.update(
315+
Some(lsp_types::NotebookDocumentCellChange {
316+
structure: Some(lsp_types::NotebookDocumentCellChangeStructure {
317+
array: lsp_types::NotebookCellArrayChange {
318+
start: 0,
319+
delete_count: 2,
320+
cells: Some(vec![
321+
lsp_types::NotebookCell {
322+
kind: lsp_types::NotebookCellKind::Code,
323+
document: create_test_url(1),
324+
metadata: None,
325+
execution_summary: None,
326+
},
327+
lsp_types::NotebookCell {
328+
kind: lsp_types::NotebookCellKind::Code,
329+
document: create_test_url(0),
330+
metadata: None,
331+
execution_summary: None,
332+
},
333+
]),
334+
},
335+
did_open: None,
336+
did_close: None,
337+
}),
338+
data: None,
339+
text_content: None,
340+
}),
341+
None,
342+
1,
343+
crate::PositionEncoding::default(),
344+
)
345+
.unwrap();
346+
347+
assert_eq!(
348+
notebook.make_ruff_notebook().source_code(),
349+
"cell = 1
350+
cell = 0
351+
cell = 2
352+
"
353+
);
354+
}
355+
}

0 commit comments

Comments
 (0)