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

Disable top-level docstring formatting for notebooks #9957

Merged
merged 3 commits into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
[
{
"source_type": "Ipynb"
}
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
"""
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
This looks like a docstring but is not in a notebook because notebooks can't be imported as a module.
Ruff should leave it as is
""";
6 changes: 3 additions & 3 deletions crates/ruff_python_formatter/src/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,9 @@ impl<'ast> PreorderVisitor<'ast> for FindEnclosingNode<'_, 'ast> {
// Don't pick potential docstrings as the closest enclosing node because `suite.rs` than fails to identify them as
// docstrings and docstring formatting won't kick in.
// Format the enclosing node instead and slice the formatted docstring from the result.
let is_maybe_docstring = node
.as_stmt_expr()
.is_some_and(|stmt| DocstringStmt::is_docstring_statement(stmt));
let is_maybe_docstring = node.as_stmt_expr().is_some_and(|stmt| {
DocstringStmt::is_docstring_statement(stmt, self.context.options().source_type())
});

if is_maybe_docstring {
return TraversalSignal::Skip;
Expand Down
32 changes: 26 additions & 6 deletions crates/ruff_python_formatter/src/statement/suite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,15 +103,19 @@ impl FormatRule<Suite, PyFormatContext<'_>> for FormatSuite {
}

SuiteKind::Function => {
if let Some(docstring) = DocstringStmt::try_from_statement(first, self.kind) {
if let Some(docstring) =
DocstringStmt::try_from_statement(first, self.kind, source_type)
{
SuiteChildStatement::Docstring(docstring)
} else {
SuiteChildStatement::Other(first)
}
}

SuiteKind::Class => {
if let Some(docstring) = DocstringStmt::try_from_statement(first, self.kind) {
if let Some(docstring) =
DocstringStmt::try_from_statement(first, self.kind, source_type)
{
if !comments.has_leading(first)
&& lines_before(first.start(), source) > 1
&& !source_type.is_stub()
Expand Down Expand Up @@ -143,7 +147,9 @@ impl FormatRule<Suite, PyFormatContext<'_>> for FormatSuite {
}
SuiteKind::TopLevel => {
if is_format_module_docstring_enabled(f.context()) {
if let Some(docstring) = DocstringStmt::try_from_statement(first, self.kind) {
if let Some(docstring) =
DocstringStmt::try_from_statement(first, self.kind, source_type)
{
SuiteChildStatement::Docstring(docstring)
} else {
SuiteChildStatement::Other(first)
Expand Down Expand Up @@ -184,7 +190,8 @@ impl FormatRule<Suite, PyFormatContext<'_>> for FormatSuite {
true
} else if is_module_docstring_newlines_enabled(f.context())
&& self.kind == SuiteKind::TopLevel
&& DocstringStmt::try_from_statement(first.statement(), self.kind).is_some()
&& DocstringStmt::try_from_statement(first.statement(), self.kind, source_type)
.is_some()
{
// Only in preview mode, insert a newline after a module level docstring, but treat
// it as a docstring otherwise. See: https://github.com/psf/black/pull/3932.
Expand Down Expand Up @@ -734,7 +741,16 @@ pub(crate) struct DocstringStmt<'a> {

impl<'a> DocstringStmt<'a> {
/// Checks if the statement is a simple string that can be formatted as a docstring
fn try_from_statement(stmt: &'a Stmt, suite_kind: SuiteKind) -> Option<DocstringStmt<'a>> {
fn try_from_statement(
stmt: &'a Stmt,
suite_kind: SuiteKind,
source_type: PySourceType,
) -> Option<DocstringStmt<'a>> {
// Notebooks don't have a concept of notebooks, never recognise them as docstrings.
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
if source_type.is_ipynb() && suite_kind == SuiteKind::TopLevel {
return None;
}

let Stmt::Expr(ast::StmtExpr { value, .. }) = stmt else {
return None;
};
Expand All @@ -752,7 +768,11 @@ impl<'a> DocstringStmt<'a> {
}
}

pub(crate) fn is_docstring_statement(stmt: &StmtExpr) -> bool {
pub(crate) fn is_docstring_statement(stmt: &StmtExpr, source_type: PySourceType) -> bool {
if source_type.is_ipynb() {
return false;
}

if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = stmt.value.as_ref() {
!value.is_implicit_concatenated()
} else {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
---
source: crates/ruff_python_formatter/tests/fixtures.rs
input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/notebook_docstring.py
---
## Input
```python
"""
This looks like a docstring but is not in a notebook because notebooks can't be imported as a module.
Ruff should leave it as is
""";
```

## Outputs
### Output 1
```
indent-style = space
line-width = 88
indent-width = 4
quote-style = Double
line-ending = LineFeed
magic-trailing-comma = Respect
docstring-code = Disabled
docstring-code-line-width = "dynamic"
preview = Disabled
target_version = Py38
source_type = Ipynb
```

```python
"""
This looks like a docstring but is not in a notebook because notebooks can't be imported as a module.
Ruff should leave it as is
""";
```



Loading