Implement StatementRangeSuccess and StatementRangeRejection#1040
Conversation
DavisVaughan
changed the title
StatementRangeError from statement_range()StatementRangeSuccess and StatementRangeRejection
|
|
||
| #[derive(Debug, Eq, PartialEq)] | ||
| struct ArkStatementRangeSuccess { | ||
| range: tree_sitter::Range, |
There was a problem hiding this comment.
It got very annoying to try and have a StatementRangeSuccess with a LSP Range deep within the roxygen2 code path that I had to convert back to a tree-sitter Range to perform the "subdocument adjustment" on.
I think it is better to have our own Ark* types here that use our internal data structure, and only convert to and from the LSP types once at the boundaries.
This feels similar to what rust-analyzer does?
There was a problem hiding this comment.
yep I think that's generally our ideal pattern in Ark/Air
(Ideally that'd be something based on TextPos, now that we've started using Rowan, but it does seem fine to have a TS type for now since that's what being consumed downstream.)
| fn adjust_roxygen_examples_rejection( | ||
| subdocument_rejection: ArkStatementRangeRejection, | ||
| row_adjustment: usize, | ||
| ) -> Option<ArkStatementRangeRejection> { | ||
| match subdocument_rejection { | ||
| ArkStatementRangeRejection::Syntax(subdocument_rejection) => { | ||
| // Adjust line number of the syntax rejection to reflect original document | ||
| Some(ArkStatementRangeRejection::Syntax( | ||
| ArkStatementRangeSyntaxRejection { | ||
| line: subdocument_rejection.line + row_adjustment as u32, | ||
| }, | ||
| )) | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Small bit of magic here, but it's not really that fancy!
| // anywhere past the first parse error, returning `None`, so that the frontend | ||
| // sends code to the console one line at a time (posit-dev/positron#5023, | ||
| // posit-dev/positron#8350). | ||
| // anywhere past the first parse error, returning a syntax rejection that points | ||
| // to the first line in this child node, which the frontend will show the user | ||
| // (posit-dev/positron#5023, posit-dev/positron#8350). | ||
| if node_has_error_or_missing(&child) { | ||
| return None; | ||
| let line = child.start_position().row as u32; | ||
| return Ok(Some(ArkStatementRangeResponse::Rejection( | ||
| ArkStatementRangeRejection::Syntax(ArkStatementRangeSyntaxRejection { line }), | ||
| ))); | ||
| } |
There was a problem hiding this comment.
Here's where we newly switch from None to a StatementRangeSyntaxRejection, which propagates up through both the normal R code path and the roxygen2 code path
| assert!(range.is_none()); | ||
| assert_matches::assert_matches!( | ||
| rejection, | ||
| ArkStatementRangeRejection::Syntax(ArkStatementRangeSyntaxRejection { line: 1 }) | ||
| ); |
There was a problem hiding this comment.
There's a lot of churn in the tests, but changes like this one in our existing tests are the meaningful bits
|
|
||
| #[derive(Debug, Eq, PartialEq)] | ||
| struct ArkStatementRangeSuccess { | ||
| range: tree_sitter::Range, |
There was a problem hiding this comment.
yep I think that's generally our ideal pattern in Ark/Air
(Ideally that'd be something based on TextPos, now that we've started using Rowan, but it does seem fine to have a TS type for now since that's what being consumed downstream.)
2 participants
Branched from #1028
Goes with posit-dev/positron#11907, and all documentation is there