Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Notebooks: Inconsistent cell indices #6673

Closed
MichaReiser opened this issue Aug 18, 2023 · 0 comments · Fixed by #7662
Closed

Notebooks: Inconsistent cell indices #6673

MichaReiser opened this issue Aug 18, 2023 · 0 comments · Fixed by #7662
Labels
bug Something isn't working

Comments

@MichaReiser
Copy link
Member

MichaReiser commented Aug 18, 2023 •
edited
Loading

Diagnostics use one based cell indices:

/home/micha/astral/test/test-notebook.ipynb:cell 1:2:8: F401 [*] `math` imported but unused
/home/micha/astral/test/test-notebook.ipynb:cell 5:1:8: F811 Redefinition of unused `random` from line 1
/home/micha/astral/test/test-notebook.ipynb:cell 5:2:8: F401 [*] `pprint` imported but unused
/home/micha/astral/test/test-notebook.ipynb:cell 7:2:4: F632 [*] Use `==` to compare constant literals
/home/micha/astral/test/test-notebook.ipynb:cell 7:3:38: F632 [*] Use `==` to compare constant literals
Found 5 errors.
[*] 4 potentially fixable with the --fix option.

Diffs use zero based cell indices

--- /home/micha/astral/test/test-notebook.ipynb:cell 0
+++ /home/micha/astral/test/test-notebook.ipynb:cell 0
@@ -1,2 +1 @@
-import random
-import math
+import random
--- /home/micha/astral/test/test-notebook.ipynb:cell 4
+++ /home/micha/astral/test/test-notebook.ipynb:cell 4
@@ -1,4 +1,3 @@
 import random
-import pprint
 
 random.randint(10, 20)
--- /home/micha/astral/test/test-notebook.ipynb:cell 6
+++ /home/micha/astral/test/test-notebook.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.

Expected

The indices to be consistent between diagnostics and the diff mode. I'm leaning towards one based indices because that's what we use for row/column too.
@MichaReiser MichaReiser added the bug Something isn't working label Aug 18, 2023
@dhruvmanila dhruvmanila mentioned this issue Sep 26, 2023
Merged
dhruvmanila added a commit that referenced this issue Sep 26, 2023
@dhruvmanila
Use 1-based cell indices consistently for Notebooks (#7662)
8165925 
## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use 1-based cell indices consistently for Notebooks astral-sh/ruff
1 participant
@MichaReiser