This repository was archived by the owner on Jul 27, 2023. It is now read-only.
impl Ranged for TextRange #20
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
This was referenced Jun 30, 2023
Merged
|
Can you extend the summary with a use case where the implementation is necessary. |
|
The original motivation was from extracting comments with differing nodes. Technically AnyNodeRef would also work there, but i think a function that requires an |
MichaReiser
approved these changes
Jul 2, 2023
zanieb
pushed a commit
that referenced
this pull request
Jul 10, 2023
This adds the missing implementation of `Ranged` for `TextRange` itself
```rust
impl Ranged for TextRange {
fn range(&self) -> TextRange {
*self
}
}
```
This allows e.g. using `has_comments` with arbitrary ranges instead of
just a node.
It also adds .venv to the .gitignore
zanieb
pushed a commit
that referenced
this pull request
Jul 17, 2023
This adds the missing implementation of `Ranged` for `TextRange` itself
```rust
impl Ranged for TextRange {
fn range(&self) -> TextRange {
*self
}
}
```
This allows e.g. using `has_comments` with arbitrary ranges instead of
just a node.
It also adds .venv to the .gitignore
zanieb
pushed a commit
that referenced
this pull request
Jul 17, 2023
This adds the missing implementation of `Ranged` for `TextRange` itself
```rust
impl Ranged for TextRange {
fn range(&self) -> TextRange {
*self
}
}
```
This allows e.g. using `has_comments` with arbitrary ranges instead of
just a node.
It also adds .venv to the .gitignore
konstin
added a commit
to astral-sh/ruff
that referenced
this pull request
Jul 18, 2023
## Summary
Previously, `StmtIf` was defined recursively as
```rust
pub struct StmtIf {
pub range: TextRange,
pub test: Box<Expr>,
pub body: Vec<Stmt>,
pub orelse: Vec<Stmt>,
}
```
Every `elif` was represented as an `orelse` with a single `StmtIf`. This
means that this representation couldn't differentiate between
```python
if cond1:
x = 1
else:
if cond2:
x = 2
```
and
```python
if cond1:
x = 1
elif cond2:
x = 2
```
It also makes many checks harder than they need to be because we have to
recurse just to iterate over an entire if-elif-else and because we're
lacking nodes and ranges on the `elif` and `else` branches.
We change the representation to a flat
```rust
pub struct StmtIf {
pub range: TextRange,
pub test: Box<Expr>,
pub body: Vec<Stmt>,
pub elif_else_clauses: Vec<ElifElseClause>,
}
pub struct ElifElseClause {
pub range: TextRange,
pub test: Option<Expr>,
pub body: Vec<Stmt>,
}
```
where `test: Some(_)` represents an `elif` and `test: None` an else.
This representation is different tradeoff, e.g. we need to allocate the
`Vec<ElifElseClause>`, the `elif`s are now different than the `if`s
(which matters in rules where want to check both `if`s and `elif`s) and
the type system doesn't guarantee that the `test: None` else is actually
last. We're also now a bit more inconsistent since all other `else`,
those from `for`, `while` and `try`, still don't have nodes. With the
new representation some things became easier, e.g. finding the `elif`
token (we can use the start of the `ElifElseClause`) and formatting
comments for if-elif-else (no more dangling comments splitting, we only
have to insert the dangling comment after the colon manually and set
`leading_alternate_branch_comments`, everything else is taken of by
having nodes for each branch and the usual placement.rs fixups).
## Merge Plan
This PR requires coordination between the parser repo and the main ruff
repo. I've split the ruff part, into two stacked PRs which have to be
merged together (only the second one fixes all tests), the first for the
formatter to be reviewed by @MichaReiser and the second for the linter
to be reviewed by @charliermarsh.
* MH: Review and merge
astral-sh/RustPython-Parser#20
* MH: Review and merge or move later in stack
astral-sh/RustPython-Parser#21
* MH: Review and approve
astral-sh/RustPython-Parser#22
* MH: Review and approve formatter PR
#5459
* CM: Review and approve linter PR
#5460
* Merge linter PR in formatter PR, fix ecosystem checks (ecosystem
checks can't run on the formatter PR and won't run on the linter PR, so
we need to merge them first)
* Merge astral-sh/RustPython-Parser#22
* Create tag in the parser, update linter+formatter PR
* Merge linter+formatter PR #5459
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
evanrittenhouse
pushed a commit
to evanrittenhouse/ruff
that referenced
this pull request
Jul 19, 2023
## Summary
Previously, `StmtIf` was defined recursively as
```rust
pub struct StmtIf {
pub range: TextRange,
pub test: Box<Expr>,
pub body: Vec<Stmt>,
pub orelse: Vec<Stmt>,
}
```
Every `elif` was represented as an `orelse` with a single `StmtIf`. This
means that this representation couldn't differentiate between
```python
if cond1:
x = 1
else:
if cond2:
x = 2
```
and
```python
if cond1:
x = 1
elif cond2:
x = 2
```
It also makes many checks harder than they need to be because we have to
recurse just to iterate over an entire if-elif-else and because we're
lacking nodes and ranges on the `elif` and `else` branches.
We change the representation to a flat
```rust
pub struct StmtIf {
pub range: TextRange,
pub test: Box<Expr>,
pub body: Vec<Stmt>,
pub elif_else_clauses: Vec<ElifElseClause>,
}
pub struct ElifElseClause {
pub range: TextRange,
pub test: Option<Expr>,
pub body: Vec<Stmt>,
}
```
where `test: Some(_)` represents an `elif` and `test: None` an else.
This representation is different tradeoff, e.g. we need to allocate the
`Vec<ElifElseClause>`, the `elif`s are now different than the `if`s
(which matters in rules where want to check both `if`s and `elif`s) and
the type system doesn't guarantee that the `test: None` else is actually
last. We're also now a bit more inconsistent since all other `else`,
those from `for`, `while` and `try`, still don't have nodes. With the
new representation some things became easier, e.g. finding the `elif`
token (we can use the start of the `ElifElseClause`) and formatting
comments for if-elif-else (no more dangling comments splitting, we only
have to insert the dangling comment after the colon manually and set
`leading_alternate_branch_comments`, everything else is taken of by
having nodes for each branch and the usual placement.rs fixups).
## Merge Plan
This PR requires coordination between the parser repo and the main ruff
repo. I've split the ruff part, into two stacked PRs which have to be
merged together (only the second one fixes all tests), the first for the
formatter to be reviewed by @MichaReiser and the second for the linter
to be reviewed by @charliermarsh.
* MH: Review and merge
astral-sh/RustPython-Parser#20
* MH: Review and merge or move later in stack
astral-sh/RustPython-Parser#21
* MH: Review and approve
astral-sh/RustPython-Parser#22
* MH: Review and approve formatter PR
astral-sh#5459
* CM: Review and approve linter PR
astral-sh#5460
* Merge linter PR in formatter PR, fix ecosystem checks (ecosystem
checks can't run on the formatter PR and won't run on the linter PR, so
we need to merge them first)
* Merge astral-sh/RustPython-Parser#22
* Create tag in the parser, update linter+formatter PR
* Merge linter+formatter PR astral-sh#5459
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
konstin
added a commit
to astral-sh/ruff
that referenced
this pull request
Jul 19, 2023
## Summary
Previously, `StmtIf` was defined recursively as
```rust
pub struct StmtIf {
pub range: TextRange,
pub test: Box<Expr>,
pub body: Vec<Stmt>,
pub orelse: Vec<Stmt>,
}
```
Every `elif` was represented as an `orelse` with a single `StmtIf`. This
means that this representation couldn't differentiate between
```python
if cond1:
x = 1
else:
if cond2:
x = 2
```
and
```python
if cond1:
x = 1
elif cond2:
x = 2
```
It also makes many checks harder than they need to be because we have to
recurse just to iterate over an entire if-elif-else and because we're
lacking nodes and ranges on the `elif` and `else` branches.
We change the representation to a flat
```rust
pub struct StmtIf {
pub range: TextRange,
pub test: Box<Expr>,
pub body: Vec<Stmt>,
pub elif_else_clauses: Vec<ElifElseClause>,
}
pub struct ElifElseClause {
pub range: TextRange,
pub test: Option<Expr>,
pub body: Vec<Stmt>,
}
```
where `test: Some(_)` represents an `elif` and `test: None` an else.
This representation is different tradeoff, e.g. we need to allocate the
`Vec<ElifElseClause>`, the `elif`s are now different than the `if`s
(which matters in rules where want to check both `if`s and `elif`s) and
the type system doesn't guarantee that the `test: None` else is actually
last. We're also now a bit more inconsistent since all other `else`,
those from `for`, `while` and `try`, still don't have nodes. With the
new representation some things became easier, e.g. finding the `elif`
token (we can use the start of the `ElifElseClause`) and formatting
comments for if-elif-else (no more dangling comments splitting, we only
have to insert the dangling comment after the colon manually and set
`leading_alternate_branch_comments`, everything else is taken of by
having nodes for each branch and the usual placement.rs fixups).
## Merge Plan
This PR requires coordination between the parser repo and the main ruff
repo. I've split the ruff part, into two stacked PRs which have to be
merged together (only the second one fixes all tests), the first for the
formatter to be reviewed by @MichaReiser and the second for the linter
to be reviewed by @charliermarsh.
* MH: Review and merge
astral-sh/RustPython-Parser#20
* MH: Review and merge or move later in stack
astral-sh/RustPython-Parser#21
* MH: Review and approve
astral-sh/RustPython-Parser#22
* MH: Review and approve formatter PR
#5459
* CM: Review and approve linter PR
#5460
* Merge linter PR in formatter PR, fix ecosystem checks (ecosystem
checks can't run on the formatter PR and won't run on the linter PR, so
we need to merge them first)
* Merge astral-sh/RustPython-Parser#22
* Create tag in the parser, update linter+formatter PR
* Merge linter+formatter PR #5459
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds the missing implementation of
RangedforTextRangeitselfThis allows e.g. using
has_commentswith arbitrary ranges instead of just a node.It also adds .venv to the .gitignore