From dbd84c947be7e603a20e22ade0f6e1e4629470a9 Mon Sep 17 00:00:00 2001
From: Dhruv Manilawala <dhruvmanila@gmail.com>
Date: Wed, 25 Oct 2023 19:31:50 +0530
Subject: [PATCH] Formatter parentheses support for `IpyEscapeCommand` (#8207)

## 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
---
 .../resources/test/fixtures/unformatted.ipynb | 35 +++++++++++++++++++
 crates/ruff_cli/tests/format.rs               | 19 +++++++++-
 .../src/expression/expr_ipy_escape_command.rs | 11 ++++++
 .../src/expression/mod.rs                     |  6 ++--
 4 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/crates/ruff_cli/resources/test/fixtures/unformatted.ipynb b/crates/ruff_cli/resources/test/fixtures/unformatted.ipynb
index 7b8ebf87a06d9..c13f24a142928 100644
--- a/crates/ruff_cli/resources/test/fixtures/unformatted.ipynb
+++ b/crates/ruff_cli/resources/test/fixtures/unformatted.ipynb
@@ -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": {
diff --git a/crates/ruff_cli/tests/format.rs b/crates/ruff_cli/tests/format.rs
index 84974d9f0bf27..bfcb69ba0f679 100644
--- a/crates/ruff_cli/tests/format.rs
+++ b/crates/ruff_cli/tests/format.rs
@@ -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 @@
diff --git a/crates/ruff_python_formatter/src/expression/expr_ipy_escape_command.rs b/crates/ruff_python_formatter/src/expression/expr_ipy_escape_command.rs
index 7e084055ac7a7..c60fc633927c1 100644
--- a/crates/ruff_python_formatter/src/expression/expr_ipy_escape_command.rs
+++ b/crates/ruff_python_formatter/src/expression/expr_ipy_escape_command.rs
@@ -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)]
@@ -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
+    }
+}
diff --git a/crates/ruff_python_formatter/src/expression/mod.rs b/crates/ruff_python_formatter/src/expression/mod.rs
index d33f05eadf6a8..48ced2960c9d1 100644
--- a/crates/ruff_python_formatter/src/expression/mod.rs
+++ b/crates/ruff_python_formatter/src/expression/mod.rs
@@ -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),
         }
     }
 }
@@ -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);