Skip to content

Commit

Permalink
Formatter parentheses support for IpyEscapeCommand (#8207)
Browse files Browse the repository at this point in the history
## Summary

This PR removes the `todo!()` around `IpyEscapeCommand` in the
formatter.

The `NeedsParentheses` trait needs to be implemented which always return
`Never`. The reason being that if an escape command is parenthesized,
then that's not parsed as an escape command. IOW, the parentheses
shouldn't be present around an escape command.

In the similar way, the `CanSkipOptionalParenthesesVisitor` will skip
this node.

## Test Plan

Updated the `unformatted.ipynb` fixture with new cells containing
IPython escape commands and the corresponding snapshot was verified.
Also, tested it out in a few open source repositories containing
notebooks (`openai/openai-cookbook`, `huggingface/notebooks`).

#### New cells in `unformatted.ipynb`

**Cell 2**
```markdown
A markdown cell
```

**Cell 3**
```python
def some_function(foo, bar):
    pass
%matplotlib inline
```

**Cell 4**
```python
foo = %pwd
def some_function(foo,bar,):
	foo = %pwd
    print(foo
	)
```

fixes: #8204
  • Loading branch information
dhruvmanila committed Oct 25, 2023
1 parent c2ec5f0 commit dbd84c9
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 4 deletions.
35 changes: 35 additions & 0 deletions crates/ruff_cli/resources/test/fixtures/unformatted.ipynb
Expand Up @@ -11,6 +11,41 @@
"maths = (numpy.arange(100)**2).sum()\n",
"stats= numpy.asarray([1,2,3,4]).median()"
]
},
{
"cell_type": "markdown",
"id": "83a0b1b8",
"metadata": {},
"source": [
"A markdown cell"
]
},
{
"cell_type": "code",
"execution_count": null,
"id": "ae12f012",
"metadata": {},
"outputs": [],
"source": [
"# A cell with IPython escape command\n",
"def some_function(foo, bar):\n",
" pass\n",
"%matplotlib inline"
]
},
{
"cell_type": "code",
"execution_count": null,
"id": "10f3bbf9",
"metadata": {},
"outputs": [],
"source": [
"foo = %pwd\n",
"def some_function(foo,bar,):\n",
" # Another cell with IPython escape command\n",
" foo = %pwd\n",
" print(foo)"
]
}
],
"metadata": {
Expand Down
19 changes: 18 additions & 1 deletion crates/ruff_cli/tests/format.rs
Expand Up @@ -511,13 +511,30 @@ fn test_diff() {
----- stdout -----
--- resources/test/fixtures/unformatted.ipynb
+++ resources/test/fixtures/unformatted.ipynb
@@ -1,3 +1,4 @@
@@ -1,12 +1,20 @@
import numpy
-maths = (numpy.arange(100)**2).sum()
-stats= numpy.asarray([1,2,3,4]).median()
+
+maths = (numpy.arange(100) ** 2).sum()
+stats = numpy.asarray([1, 2, 3, 4]).median()
# A cell with IPython escape command
def some_function(foo, bar):
pass
+
+
%matplotlib inline
foo = %pwd
-def some_function(foo,bar,):
+
+
+def some_function(
+ foo,
+ bar,
+):
# Another cell with IPython escape command
foo = %pwd
print(foo)
--- resources/test/fixtures/unformatted.py
+++ resources/test/fixtures/unformatted.py
@@ -1,3 +1,3 @@
Expand Down
@@ -1,6 +1,7 @@
use ruff_python_ast::ExprIpyEscapeCommand;
use ruff_text_size::Ranged;

use crate::expression::parentheses::{NeedsParentheses, OptionalParentheses};
use crate::prelude::*;

#[derive(Default)]
Expand All @@ -11,3 +12,13 @@ impl FormatNodeRule<ExprIpyEscapeCommand> for FormatExprIpyEscapeCommand {
source_text_slice(item.range()).fmt(f)
}
}

impl NeedsParentheses for ExprIpyEscapeCommand {
fn needs_parentheses(
&self,
_parent: ruff_python_ast::AnyNodeRef,
_context: &PyFormatContext,
) -> OptionalParentheses {
OptionalParentheses::Never
}
}
6 changes: 3 additions & 3 deletions crates/ruff_python_formatter/src/expression/mod.rs
Expand Up @@ -476,7 +476,7 @@ impl NeedsParentheses for Expr {
Expr::List(expr) => expr.needs_parentheses(parent, context),
Expr::Tuple(expr) => expr.needs_parentheses(parent, context),
Expr::Slice(expr) => expr.needs_parentheses(parent, context),
Expr::IpyEscapeCommand(_) => todo!(),
Expr::IpyEscapeCommand(expr) => expr.needs_parentheses(parent, context),
}
}
}
Expand Down Expand Up @@ -718,8 +718,8 @@ impl<'input> CanOmitOptionalParenthesesVisitor<'input> {
| Expr::Constant(_)
| Expr::Starred(_)
| Expr::Name(_)
| Expr::Slice(_) => {}
Expr::IpyEscapeCommand(_) => todo!(),
| Expr::Slice(_)
| Expr::IpyEscapeCommand(_) => {}
};

walk_expr(self, expr);
Expand Down

0 comments on commit dbd84c9

Please sign in to comment.