Skip to content

Commit

Permalink
Use 1-based cell indices consistently for Notebooks (#7662)
Browse files Browse the repository at this point in the history
## Summary

This PR fixes the bug where the cell indices displayed in the `--diff` output
and the ones in the normal output were different. This was due to the fact that
the `--diff` output was using the `enumerate` function to iterate over
the cells which starts at 0.

## Test Plan

Ran the following command with and without the `--diff` flag:

```console
cargo run --bin ruff -- check --no-cache --isolated ~/playground/ruff/notebooks/test.ipynb
```

### `main`

<details><summary>Diagnostics output:</summary>
<p>

```console
$ cargo run --bin ruff -- check --no-cache --isolated ~/playground/ruff/notebooks/test.ipynb       
/Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 3:2:8: F401 [*] `math` imported but unused
/Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 5:1:8: F811 Redefinition of unused `random` from line 1
/Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 5:2:8: F401 [*] `pprint` imported but unused
/Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 6:2:4: F632 [*] Use `==` to compare constant literals
/Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 6:3:38: F632 [*] Use `==` to compare constant literals
Found 5 errors.
[*] 4 potentially fixable with the --fix option.
```

</p>
</details>

<details><summary>Diff output:</summary>
<p>

```console
$ cargo run --bin ruff -- check --no-cache --isolated ~/playground/ruff/notebooks/test.ipynb --diff
--- /Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 2
+++ /Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 2
@@ -1,2 +1 @@
-import random
-import math
+import random
--- /Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 4
+++ /Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 4
@@ -1,4 +1,3 @@
 import random
-import pprint
 
 random.randint(10, 20)
--- /Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 5
+++ /Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 5
@@ -1,3 +1,3 @@
 foo = 1
-if foo is 2:
-    raise ValueError(f"Invalid foo: {foo is 1}")
+if foo == 2:
+    raise ValueError(f"Invalid foo: {foo == 1}")

Would fix 4 errors.
```

</p>
</details> 

### `dhruv/consistent-cell-indices`

<details><summary>Diagnostic output:</summary>
<p>

```console
$ cargo run --bin ruff -- check --no-cache --isolated ~/playground/ruff/notebooks/test.ipynb           
/Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 3:2:8: F401 [*] `math` imported but unused
/Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 5:1:8: F811 Redefinition of unused `random` from line 1
/Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 5:2:8: F401 [*] `pprint` imported but unused
/Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 6:2:4: F632 [*] Use `==` to compare constant literals
/Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 6:3:38: F632 [*] Use `==` to compare constant literals
Found 5 errors.
[*] 4 potentially fixable with the --fix option.
```

</p>
</details> 

<details><summary>Diff output:</summary>
<p>

```console
$ cargo run --bin ruff -- check --no-cache --isolated ~/playground/ruff/notebooks/test.ipynb --diff
--- /Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 3
+++ /Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 3
@@ -1,2 +1 @@
-import random
-import math
+import random
--- /Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 5
+++ /Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 5
@@ -1,4 +1,3 @@
 import random
-import pprint
 
 random.randint(10, 20)
--- /Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 6
+++ /Users/dhruv/playground/ruff/notebooks/test.ipynb:cell 6
@@ -1,3 +1,3 @@
 foo = 1
-if foo is 2:
-    raise ValueError(f"Invalid foo: {foo is 1}")
+if foo == 2:
+    raise ValueError(f"Invalid foo: {foo == 1}")

Would fix 4 errors.
```

</p>
</details> 

fixes: #6673
  • Loading branch information
dhruvmanila committed Sep 26, 2023
1 parent 0a73784 commit 8165925
Showing 1 changed file with 3 additions and 4 deletions.
7 changes: 3 additions & 4 deletions crates/ruff_cli/src/diagnostics.rs
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

0 comments on commit 8165925

Please sign in to comment.