Skip to content

Formatter parentheses support for IpyEscapeCommand#8207

Merged
dhruvmanila merged 2 commits intomainfrom
dhruv/formatter-ipy-todo
Oct 25, 2023
Merged

Formatter parentheses support for IpyEscapeCommand#8207
dhruvmanila merged 2 commits intomainfrom
dhruv/formatter-ipy-todo

Conversation

@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Oct 25, 2023

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

A markdown cell

Cell 3

def some_function(foo, bar):
    pass
%matplotlib inline

Cell 4

foo = %pwd
def some_function(foo,bar,):
	foo = %pwd
    print(foo
	)

fixes: #8204

@dhruvmanila
Copy link
Member Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@dhruvmanila dhruvmanila force-pushed the dhruv/formatter-source-type branch from c8feeda to 6012c5d Compare October 25, 2023 09:51
@dhruvmanila dhruvmanila force-pushed the dhruv/formatter-ipy-todo branch from b0a7908 to 15984dc Compare October 25, 2023 09:52
@dhruvmanila dhruvmanila marked this pull request as ready for review October 25, 2023 10:28
@dhruvmanila dhruvmanila added bug Something isn't working formatter Related to the formatter labels Oct 25, 2023
@dhruvmanila dhruvmanila requested review from MichaReiser and konstin and removed request for MichaReiser October 25, 2023 10:28
Copy link
Member

@MichaReiser MichaReiser left a 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?

@dhruvmanila dhruvmanila force-pushed the dhruv/formatter-ipy-todo branch from 15984dc to 7dd63e1 Compare October 25, 2023 13:27
@dhruvmanila
Copy link
Member Author

Can we add an E2E test (CLI test) in the parent PR that tests ruff format with a notebook that contains ipython commands?

Do you mean in this PR? I've added it.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 25, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Base automatically changed from dhruv/formatter-source-type to main October 25, 2023 13:50
@dhruvmanila dhruvmanila reopened this Oct 25, 2023
@dhruvmanila dhruvmanila enabled auto-merge (squash) October 25, 2023 13:52
@dhruvmanila dhruvmanila merged commit dbd84c9 into main Oct 25, 2023
@dhruvmanila dhruvmanila deleted the dhruv/formatter-ipy-todo branch October 25, 2023 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working formatter Related to the formatter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failed to format notebook: source contains syntax errors: ParseError

3 participants