From 6bbabceead840e0d232950a1ed1e8f02cdafb92c Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Thu, 7 Dec 2023 14:15:43 -0600 Subject: [PATCH] Allow transparent cell magics (#8911) ## Summary This PR updates the logic for `is_magic_cell` to include certain cell magics. These cell magics would contain Python code following the line defining the command. The code could define a variable which can then be referenced in other cells. Currently, we would ignore the cell completely leading to undefined-name violation. As discussed in https://github.com/astral-sh/ruff/issues/8354#issuecomment-1832221009 ## Test Plan Add new test case to validate this scenario. --- ...er__linter__tests__ipy_escape_command.snap | 15 +++++++ .../fixtures/jupyter/cell/cell_magic.json | 7 ++- .../jupyter/cell/valid_cell_magic.json | 11 +++++ .../fixtures/jupyter/ipy_escape_command.ipynb | 12 +++++ .../jupyter/ipy_escape_command_expected.ipynb | 15 ++++++- crates/ruff_notebook/src/cell.rs | 45 ++++++++++++++++++- crates/ruff_notebook/src/notebook.rs | 1 + 7 files changed, 102 insertions(+), 4 deletions(-) create mode 100644 crates/ruff_notebook/resources/test/fixtures/jupyter/cell/valid_cell_magic.json diff --git a/crates/ruff_linter/src/snapshots/ruff_linter__linter__tests__ipy_escape_command.snap b/crates/ruff_linter/src/snapshots/ruff_linter__linter__tests__ipy_escape_command.snap index 58aa72d7da..8c1a547e85 100644 --- a/crates/ruff_linter/src/snapshots/ruff_linter__linter__tests__ipy_escape_command.snap +++ b/crates/ruff_linter/src/snapshots/ruff_linter__linter__tests__ipy_escape_command.snap @@ -19,5 +19,20 @@ ipy_escape_command.ipynb:cell 1:5:8: F401 [*] `os` imported but unused 5 |-import os 6 5 | 7 6 | _ = math.pi +8 7 | %%timeit + +ipy_escape_command.ipynb:cell 2:2:8: F401 [*] `sys` imported but unused + | +1 | %%timeit +2 | import sys + | ^^^ F401 + | + = help: Remove unused import: `sys` + +ℹ Safe fix +6 6 | +7 7 | _ = math.pi +8 8 | %%timeit +9 |-import sys diff --git a/crates/ruff_notebook/resources/test/fixtures/jupyter/cell/cell_magic.json b/crates/ruff_notebook/resources/test/fixtures/jupyter/cell/cell_magic.json index ef68b202e6..e0de8c0241 100644 --- a/crates/ruff_notebook/resources/test/fixtures/jupyter/cell/cell_magic.json +++ b/crates/ruff_notebook/resources/test/fixtures/jupyter/cell/cell_magic.json @@ -4,5 +4,10 @@ "id": "1", "metadata": {}, "outputs": [], - "source": ["%%timeit\n", "print('hello world')"] + "source": [ + "%%script bash\n", + "for i in 1 2 3; do\n", + " echo $i\n", + "done" + ] } diff --git a/crates/ruff_notebook/resources/test/fixtures/jupyter/cell/valid_cell_magic.json b/crates/ruff_notebook/resources/test/fixtures/jupyter/cell/valid_cell_magic.json new file mode 100644 index 0000000000..2cb89fa63b --- /dev/null +++ b/crates/ruff_notebook/resources/test/fixtures/jupyter/cell/valid_cell_magic.json @@ -0,0 +1,11 @@ +{ + "execution_count": null, + "cell_type": "code", + "id": "1", + "metadata": {}, + "outputs": [], + "source": [ + "%%timeit\n", + "print('hello world')" + ] +} diff --git a/crates/ruff_notebook/resources/test/fixtures/jupyter/ipy_escape_command.ipynb b/crates/ruff_notebook/resources/test/fixtures/jupyter/ipy_escape_command.ipynb index 5e9b10bb7b..6937096cc0 100644 --- a/crates/ruff_notebook/resources/test/fixtures/jupyter/ipy_escape_command.ipynb +++ b/crates/ruff_notebook/resources/test/fixtures/jupyter/ipy_escape_command.ipynb @@ -26,6 +26,18 @@ "%%timeit\n", "import sys" ] + }, + { + "cell_type": "code", + "execution_count": null, + "id": "36dedfd1-6c03-4894-bea6-6c1687b82b3c", + "metadata": {}, + "outputs": [], + "source": [ + "%%random\n", + "# This cell is ignored\n", + "import pathlib" + ] } ], "metadata": { diff --git a/crates/ruff_notebook/resources/test/fixtures/jupyter/ipy_escape_command_expected.ipynb b/crates/ruff_notebook/resources/test/fixtures/jupyter/ipy_escape_command_expected.ipynb index 8419f031e7..6a5eebc05f 100644 --- a/crates/ruff_notebook/resources/test/fixtures/jupyter/ipy_escape_command_expected.ipynb +++ b/crates/ruff_notebook/resources/test/fixtures/jupyter/ipy_escape_command_expected.ipynb @@ -22,8 +22,19 @@ "metadata": {}, "outputs": [], "source": [ - "%%timeit\n", - "import sys" + "%%timeit" + ] + }, + { + "cell_type": "code", + "execution_count": null, + "id": "4b6d7faa-72b3-4087-8670-fe6d35e41fb6", + "metadata": {}, + "outputs": [], + "source": [ + "%%random\n", + "# This cell is ignored\n", + "import pathlib" ] } ], diff --git a/crates/ruff_notebook/src/cell.rs b/crates/ruff_notebook/src/cell.rs index d80ef336de..caa4cb3204 100644 --- a/crates/ruff_notebook/src/cell.rs +++ b/crates/ruff_notebook/src/cell.rs @@ -170,7 +170,50 @@ impl Cell { } // Detect cell magics (which operate on multiple lines). - lines.any(|line| line.trim_start().starts_with("%%")) + lines.any(|line| { + line.split_whitespace().next().is_some_and(|first| { + if first.len() < 2 { + return false; + } + let (token, command) = first.split_at(2); + // These cell magics are special in that the lines following them are valid + // Python code and the variables defined in that scope are available to the + // rest of the notebook. + // + // For example: + // + // Cell 1: + // ```python + // x = 1 + // ``` + // + // Cell 2: + // ```python + // %%time + // y = x + // ``` + // + // Cell 3: + // ```python + // print(y) # Here, `y` is available. + // ``` + // + // This is to avoid false positives when these variables are referenced + // elsewhere in the notebook. + token == "%%" + && !matches!( + command, + "capture" + | "debug" + | "prun" + | "pypy" + | "python" + | "python3" + | "time" + | "timeit" + ) + }) + }) } } diff --git a/crates/ruff_notebook/src/notebook.rs b/crates/ruff_notebook/src/notebook.rs index a6714fa12b..7c9e8356b7 100644 --- a/crates/ruff_notebook/src/notebook.rs +++ b/crates/ruff_notebook/src/notebook.rs @@ -426,6 +426,7 @@ mod tests { #[test_case(Path::new("code_and_magic.json"), true; "code_and_magic")] #[test_case(Path::new("only_code.json"), true; "only_code")] #[test_case(Path::new("cell_magic.json"), false; "cell_magic")] + #[test_case(Path::new("valid_cell_magic.json"), true; "valid_cell_magic")] #[test_case(Path::new("automagic.json"), false; "automagic")] #[test_case(Path::new("automagics.json"), false; "automagics")] #[test_case(Path::new("automagic_before_code.json"), false; "automagic_before_code")]