-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Formatter parentheses support for IpyEscapeCommand
#8207
Conversation
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
c8feeda
to
6012c5d
Compare
b0a7908
to
15984dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Can we add an E2E test (CLI test) in the parent PR that tests ruff format
with a notebook that contains ipython commands?
15984dc
to
7dd63e1
Compare
Do you mean in this PR? I've added it. |
PR Check ResultsEcosystem✅ ecosystem check detected no changes. |
Summary
This PR removes the
todo!()
aroundIpyEscapeCommand
in the formatter.The
NeedsParentheses
trait needs to be implemented which always returnNever
. 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
Cell 3
Cell 4
fixes: #8204