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.

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

Update F841 autofix to not remove line magic expr #6141

Merged
merged 22 commits into from
Aug 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
64dee42
Use Jupyter mode for the parser with Notebook files
dhruvmanila Jul 26, 2023
67183d6
Add magics to import sorting tests for Jupyter Notebook
dhruvmanila Jul 26, 2023
9d14de7
Fix imports
dhruvmanila Jul 26, 2023
4c5dd4e
Actually resolve merge conflicts
dhruvmanila Jul 26, 2023
96dd177
Use Jupyter mode for lexing
dhruvmanila Jul 26, 2023
adb790a
Use `is_jupyter_notebook` param
dhruvmanila Jul 27, 2023
0b958d6
Return both original and updated `SourceKind` in tests
dhruvmanila Jul 27, 2023
2e953cd
Update test case to include cell magic
dhruvmanila Jul 27, 2023
9bc3764
Remove leftover imports from rebase
dhruvmanila Jul 28, 2023
0d27e56
Use `PySourceType` for detecting lexing/parsing mode
dhruvmanila Jul 28, 2023
9d94cd6
Run cargo fmt
dhruvmanila Jul 28, 2023
bc9ec50
Fix clippy warnings
dhruvmanila Jul 28, 2023
689267b
Use `source_type` (leftover from rebase)
dhruvmanila Aug 2, 2023
f453a7c
Merge branch 'main' into dhruv/jupyter-cell-magic
dhruvmanila Aug 4, 2023
65fe5cd
Fix CI (missed in the merge)
dhruvmanila Aug 4, 2023
66edc7f
Run `cargo fmt`
dhruvmanila Aug 4, 2023
c822cb2
Merge branch 'main' into dhruv/jupyter-cell-magic
dhruvmanila Aug 4, 2023
8a925c7
Merge branch 'main' into dhruv/jupyter-cell-magic
dhruvmanila Aug 5, 2023
ba1fa98
Add `AsMode` trait for `PySourceType`
dhruvmanila Aug 5, 2023
851767e
Run `cargo fmt`
dhruvmanila Aug 5, 2023
fd2874a
Update `F841` autofix to not remove line magic expr
dhruvmanila Jul 27, 2023
17ced84
Merge branch 'main' into dhruv/F841-line-magic-expr
dhruvmanila Aug 5, 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
49 changes: 49 additions & 0 deletions crates/ruff/resources/test/fixtures/jupyter/unused_variable.ipynb
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
{
"cells": [
{
"cell_type": "code",
"execution_count": null,
"id": "a0efffbc-85f1-4513-bf49-5387ec3a2a4e",
"metadata": {},
"outputs": [],
"source": [
"def f():\n",
" foo1 = %matplotlib --list\n",
" foo2: list[str] = %matplotlib --list"
]
},
{
"cell_type": "code",
"execution_count": null,
"id": "6e0b2b50-43f2-4f59-951d-9404dd560ae4",
"metadata": {},
"outputs": [],
"source": [
"def f():\n",
" bar1 = !pwd\n",
" bar2: str = !pwd"
]
}
],
"metadata": {
"kernelspec": {
"display_name": "Python (ruff)",
"language": "python",
"name": "ruff"
},
"language_info": {
"codemirror_mode": {
"name": "ipython",
"version": 3
},
"file_extension": ".py",
"mimetype": "text/x-python",
"name": "python",
"nbconvert_exporter": "python",
"pygments_lexer": "ipython3",
"version": "3.11.3"
}
},
"nbformat": 4,
"nbformat_minor": 5
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
{
"cells": [
{
"cell_type": "code",
"execution_count": null,
"id": "24426ef2-046c-453e-b809-05b56e7355e0",
"metadata": {},
"outputs": [],
"source": [
"def f():\n",
" %matplotlib --list\n",
" %matplotlib --list"
]
},
{
"cell_type": "code",
"execution_count": null,
"id": "3d98fdae-b86b-476e-b4db-9d3ce5562682",
"metadata": {},
"outputs": [],
"source": [
"def f():\n",
" !pwd\n",
" !pwd"
]
}
],
"metadata": {
"kernelspec": {
"display_name": "Python (ruff)",
"language": "python",
"name": "ruff"
},
"language_info": {
"codemirror_mode": {
"name": "ipython",
"version": 3
},
"file_extension": ".py",
"mimetype": "text/x-python",
"name": "python",
"nbconvert_exporter": "python",
"pygments_lexer": "ipython3",
"version": "3.11.3"
}
},
"nbformat": 4,
"nbformat_minor": 5
}
12 changes: 12 additions & 0 deletions crates/ruff/src/jupyter/notebook.rs
Original file line number Diff line number Diff line change
Expand Up @@ -582,6 +582,18 @@ print("after empty cells")
Ok(())
}

#[test]
fn test_unused_variable() -> Result<()> {
let path = "unused_variable.ipynb".to_string();
let (diagnostics, source_kind, _) = test_notebook_path(
&path,
Path::new("unused_variable_expected.ipynb"),
&settings::Settings::for_rule(Rule::UnusedVariable),
)?;
assert_messages!(diagnostics, path, source_kind);
Ok(())
}

#[test]
fn test_json_consistency() -> Result<()> {
let path = "before_fix.ipynb".to_string();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
---
source: crates/ruff/src/jupyter/notebook.rs
---
unused_variable.ipynb:cell 1:2:5: F841 [*] Local variable `foo1` is assigned to but never used
|
1 | def f():
2 | foo1 = %matplotlib --list
| ^^^^ F841
3 | foo2: list[str] = %matplotlib --list
|
= help: Remove assignment to unused variable `foo1`

Suggested fix
1 1 | def f():
2 |- foo1 = %matplotlib --list
2 |+ %matplotlib --list
3 3 | foo2: list[str] = %matplotlib --list
4 4 | def f():
5 5 | bar1 = !pwd

unused_variable.ipynb:cell 1:3:5: F841 [*] Local variable `foo2` is assigned to but never used
|
1 | def f():
2 | foo1 = %matplotlib --list
3 | foo2: list[str] = %matplotlib --list
| ^^^^ F841
|
= help: Remove assignment to unused variable `foo2`

Suggested fix
1 1 | def f():
2 2 | foo1 = %matplotlib --list
3 |- foo2: list[str] = %matplotlib --list
3 |+ %matplotlib --list
4 4 | def f():
5 5 | bar1 = !pwd
6 6 | bar2: str = !pwd

unused_variable.ipynb:cell 2:2:5: F841 [*] Local variable `bar1` is assigned to but never used
|
1 | def f():
2 | bar1 = !pwd
| ^^^^ F841
3 | bar2: str = !pwd
|
= help: Remove assignment to unused variable `bar1`

Suggested fix
2 2 | foo1 = %matplotlib --list
3 3 | foo2: list[str] = %matplotlib --list
4 4 | def f():
5 |- bar1 = !pwd
5 |+ !pwd
6 6 | bar2: str = !pwd

unused_variable.ipynb:cell 2:3:5: F841 [*] Local variable `bar2` is assigned to but never used
|
1 | def f():
2 | bar1 = !pwd
3 | bar2: str = !pwd
| ^^^^ F841
|
= help: Remove assignment to unused variable `bar2`

Suggested fix
3 3 | foo2: list[str] = %matplotlib --list
4 4 | def f():
5 5 | bar1 = !pwd
6 |- bar2: str = !pwd
6 |+ !pwd


1 change: 1 addition & 0 deletions crates/ruff_python_ast/src/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ where
| Expr::Subscript(_)
| Expr::Yield(_)
| Expr::YieldFrom(_)
| Expr::LineMagic(_)
Copy link
Member

@MichaReiser MichaReiser Aug 3, 2023

Choose a reason for hiding this comment

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

I believe there have been a couple of PRs that all required the same change. Is there a more sustainable approach to this to avoid repeating this process in the future when adding a new expression? Could we e.g. have a helper and re-use it for all the expressions that we don't want to be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct me if I'm wrong but I think this is defined in a helper function itself (contains_effect) or are you talking about something else? I'll merge this for now.

)
})
}
Expand Down