Preserve trailing semicolon for Notebooks#8590
Conversation
|
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
|
783e240 to
3ed4a2f
Compare
| #[test] | ||
| fn test_notebook_trailing_semicolon() { | ||
| let fixtures = Path::new("resources").join("test").join("fixtures"); | ||
| let unformatted = fs::read(fixtures.join("trailing_semicolon.ipynb")).unwrap(); | ||
| assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) |
There was a problem hiding this comment.
I'm not sure if this is the correct place for this test case. We would need to refactor the formatter test suite to include notebook files.
| SuiteKind::TopLevel => NodeLevel::TopLevel, | ||
| SuiteKind::TopLevel => NodeLevel::TopLevel( | ||
| iter.clone() | ||
| .next() | ||
| .map_or(TopLevelStatementPosition::Last, |_| { | ||
| TopLevelStatementPosition::Other | ||
| }), | ||
| ), |
There was a problem hiding this comment.
We could avoid cloning the iterator by making it peekable but that involves updating the function signature wherever this iterator is being passed. This is because it expects std::slice::Iter.
There was a problem hiding this comment.
This clone is effectively free (we just clone the slice reference), so this is fine. The peakable things with the function signatures is annoying though, i had also already hit that
konstin
left a comment
There was a problem hiding this comment.
Would it simplify the implementation to insert the semicolon at
instead? Then we shouldn't need TopLevelStatementPosition
| SuiteKind::TopLevel => NodeLevel::TopLevel, | ||
| SuiteKind::TopLevel => NodeLevel::TopLevel( | ||
| iter.clone() | ||
| .next() | ||
| .map_or(TopLevelStatementPosition::Last, |_| { | ||
| TopLevelStatementPosition::Other | ||
| }), | ||
| ), |
There was a problem hiding this comment.
This clone is effectively free (we just clone the slice reference), so this is fine. The peakable things with the function signatures is annoying though, i had also already hit that
I'm assuming what you're recommending is to check at the linked position if this is the last statement and then preserve the semicolon, if any. Is this correct? I think that'll involve matching against the relevant nodes where the semicolon needs to be preserved as it's only required for a subset of nodes - |
MichaReiser
left a comment
There was a problem hiding this comment.
I prefer to avoid using the context whenever possible because it is harder to reason about where the context value is set/changed.
An alternative to the context would have been to parameterized FormatStmt, FormatStmtAssign etc (by implementing FormatWithOptions) and explicitly pass a value whether it is the last statement.
| SuiteKind::TopLevel => NodeLevel::TopLevel( | ||
| iter.clone() | ||
| .next() | ||
| .map_or(TopLevelStatementPosition::Last, |_| { | ||
| TopLevelStatementPosition::Other | ||
| }), | ||
| ), |
There was a problem hiding this comment.
I think this can be simplified to
SuiteKind::TopLevel => NodeLevel::TopLevel(if statements.len() < 2 {
TopLevelStatementPosition::Last
} else {
TopLevelStatementPosition::Other
}),There was a problem hiding this comment.
Yes, that can be done.
I thought about this but I felt like this kind of information is suited well for the context. And, it already provided the node level information. |
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:
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