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

formatter(Jupyter): ending semicolons valid in Jupyter #8254

Closed
henryiii opened this issue Oct 26, 2023 · 10 comments · Fixed by #8590
Closed

formatter(Jupyter): ending semicolons valid in Jupyter #8254

henryiii opened this issue Oct 26, 2023 · 10 comments · Fixed by #8590
Assignees
Labels
bug Something isn't working formatter Related to the formatter

Comments

@henryiii
Copy link
Contributor

henryiii commented Oct 26, 2023

The Jupyter formatter has a couple of issues when running on notebooks.

One is it removes semicolons at the end of lines. black[jupyter] does not remove semicolons at the end of lines when running on a notebook, since it's used to mark output as hidden, for example when plotting. plt.plot(); will show the plot but not print out the repr for the Artists that the plot method returns.

Edit: On the last line of a cell. Related: #7300 which is for the linter. But that's rule based, so it's easy to turn it off for *.ipynb files, while there isn't a workaround for the formatter, making it a blocker.

(Fixed in main) The other is that it can't read a notebook that black and ruff-lint are able to read. The error is:

error: Failed to format docs/examples/HistDemo.ipynb: source contains syntax errors: ParseError { error: UnrecognizedToken(Percent, None), offset: 0, source_path: "<filename>" }

I'm guessing it's choking on a line magic, perhaps? The file does have this line:

%config InteractiveShell.ast_node_interactivity="last_expr_or_assign"

Which is the only % in the file.

@charliermarsh
Copy link
Member

Thanks!

@charliermarsh charliermarsh added bug Something isn't working formatter Related to the formatter labels Oct 26, 2023
@dhruvmanila
Copy link
Member

Re line magic, it's solved #8204, sorry for the churn!

@henryiii
Copy link
Contributor Author

Great! Probably should have checked main. So just simicolons left.

@henryiii henryiii changed the title formatter(Jupyter): semicolons and percents formatter(Jupyter): ending semicolons valid in Jupyter Oct 26, 2023
@dhruvmanila
Copy link
Member

Related: #7300

@MichaReiser MichaReiser added this to the Formatter: Stable milestone Oct 27, 2023
@MichaReiser
Copy link
Member

Is my assumption correct that this only applies to expression statements or are there other statements for which semicolons need to be preserved?

@henryiii
Copy link
Contributor Author

henryiii commented Oct 27, 2023

IPython has several modes. If you set:

%config InteractiveShell.ast_node_interactivity="last_expr_or_assign"

Then either val = expr or just expr will print if it's the final statement in the cell, unless there's a semicolon (see, for example, https://hist.readthedocs.io/en/latest/examples/HistDemo.html). By default, only expr will display, and val = expr won't. I'd say simply allowing a final semicolon in cells on expressions or assignment expressions would be fine.

IIRC a.b = and a[b] = statements don't display even in the enhanced mode, but could be wrong.

@MichaReiser
Copy link
Member

Note, there's now a helper function to extract the trailing semicolon. We can use it in the assignment (including augmented and annotated) and expression statement formatting

pub(super) fn trailing_semicolon(node: AnyNodeRef, source: &str) -> Option<TextRange> {
debug_assert!(node.is_statement());
let tokenizer = SimpleTokenizer::starts_at(node.end(), source);
let next_token = tokenizer
.take_while(|token| !token.kind().is_comment())
.find(|token| !token.kind().is_trivia());
if let Some(SimpleToken {
kind: SimpleTokenKind::Semi,
range,
}) = next_token
{
Some(range)
} else {
None
}
}

@dhruvmanila
Copy link
Member

Black's implementation for reference: https://github.com/psf/black/blob/c54c213d6a3132986feede0cf0525f5bae5b43d6/src/black/handle_ipynb_magics.py#L73-L102. It seems to be removing them for all cases.

@henryiii
Copy link
Contributor Author

henryiii commented Nov 3, 2023

The function after that puts them back - not sure why you'd need to remove it and then put it back, since the normal formatter also removes them - just checking to see if it's there and then putting it back seems like it would also work. Anyway, what cases are not covered by assignments and expressions? Blocks can't end lines, for example. Simicolons aren't needed for import statements - I wonder if black keeps them, actually. :)

@MichaReiser
Copy link
Member

Black's implementation for reference: psf/black@c54c213/src/black/handle_ipynb_magics.py#L73-L102. It seems to be removing them for all cases.

Nice find. Removing them and adding them back sounds complicated. We can just preserve them 😄

@dhruvmanila dhruvmanila self-assigned this Nov 9, 2023
dhruvmanila added a commit that referenced this issue Nov 10, 2023
## Summary

This PR updates the formatter to preserve trailing semicolon for Jupyter
Notebooks.

The motivation behind the change is that semicolons in notebooks are
typically used to hide the output, for example when plotting. This is
highlighted in the linked issue.

The conditions required as to when the trailing semicolon should be
preserved are:
1. It should be a top-level statement which is last in the module.
2. For statement, it can be either assignment, annotated assignment, or
augmented assignment. Here, the target should only be a single
identifier i.e., multiple assignments or tuple unpacking isn't
considered.
3. For expression, it can be any.

## Test Plan

Add a new integration test in `ruff_cli`. The test notebook basically
acts as a document as to which trailing semicolons are to be preserved.

fixes: #8254
henryiii added a commit to scikit-hep/hist that referenced this issue Nov 17, 2023
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 a pull request may close this issue.

4 participants