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

Allow transparent cell magics #8911

Merged
merged 2 commits into from
Dec 7, 2023
Merged

Allow transparent cell magics #8911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f71a12
Allow transparent cell magics
dhruvmanila Nov 29, 2023
11b7673
Update existing test case
dhruvmanila Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions crates/ruff_linter/src/snapshots/ruff_linter__linter__tests__ipy_escape_command.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


7 changes: 6 additions & 1 deletion crates/ruff_notebook/resources/test/fixtures/jupyter/cell/cell_magic.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
]
}
11 changes: 11 additions & 0 deletions crates/ruff_notebook/resources/test/fixtures/jupyter/cell/valid_cell_magic.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"execution_count": null,
"cell_type": "code",
"id": "1",
"metadata": {},
"outputs": [],
"source": [
"%%timeit\n",
"print('hello world')"
]
}
12 changes: 12 additions & 0 deletions crates/ruff_notebook/resources/test/fixtures/jupyter/ipy_escape_command.ipynb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
15 changes: 13 additions & 2 deletions crates/ruff_notebook/resources/test/fixtures/jupyter/ipy_escape_command_expected.ipynb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
]
}
],
Expand Down
45 changes: 44 additions & 1 deletion crates/ruff_notebook/src/cell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So how does the parser handle this case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parser would parse the first line %%time as a cell magic statement and the second line as an assignment statement.

This is technically a bug in the parser as both lines should be a single node instead but as we always ignored cell magic, it never came up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does the cell magic end? Does it just include the %%time regardless of what follows? Like how would the parser deal with:

%%time x = 1
y = x

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each statement is a single node, so %%time x = 1 would a single node and y = x would be an assignment statement. This code would raise undefined name for x. This is intentional as that requires support for inline parsing which might require more thinking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mostly wondering how the parser knows where the magic statement ends.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it's the newline. The lexer consumes everything till the newline and considers this as part of the command.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, makes sense. Could there be continuations though?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but that is ignored. So,

%%time \
	x = 1
y = x

will parse in same way as your example.

// ```
//
// 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"
)
})
})
}
}

Expand Down
1 change: 1 addition & 0 deletions crates/ruff_notebook/src/notebook.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down
Loading