Skip to content

Migrate document to use YARP - #863

Merged
andyw8 merged 10 commits into
yarpfrom
andyw8/migrate-document-to-use-yarp
Aug 9, 2023
Merged

Migrate document to use YARP#863
andyw8 merged 10 commits into
yarpfrom
andyw8/migrate-document-to-use-yarp

Conversation

@andyw8

@andyw8andyw8 commented Aug 8, 2023

Copy link
Copy Markdown
Contributor

Cherry-pick of @vinistock's PR from another branch. I then updated the tests.

bin/test test/document_test.rb is passing

@github-actions

Copy link
Copy Markdown
Contributor

Comment threadtest/document_test.rb Outdated
end

def test_parsed_returns_false_when_parsing_fails
def test_parsed_returns_true_when_parsing_fails

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since YARP is able to parse code with syntax errors, I've changed the behaviour here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can get rid of this one when removing parsed?.

@andyw8
andyw8force-pushed the andyw8/migrate-document-to-use-yarp branch from 2e92719 to 4f28646CompareAugust 9, 2023 13:43
@uri = T.let(uri, URI::Generic)
@unparsed_edits = T.let([], T::Array[EditShape])
@syntax_error = T.let(false, T::Boolean)
@tree = T.let(SyntaxTree.parse(@source), T.nilable(SyntaxTree::Node))

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are introducing @parse_result, I eliminated @tree, to reduce the number of state variables needed.

@github-actions

Copy link
Copy Markdown
Contributor

2 similar comments
@github-actions

Copy link
Copy Markdown
Contributor

@github-actions

Copy link
Copy Markdown
Contributor

Comment threadlib/ruby_lsp/document.rb Outdated
sig { returns(T::Boolean) }
def parsed?
!@tree.nil?
!@parse_result.value.nil?

@andyw8andyw8Aug 9, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vinistock you had removed this method but I re-added (since some other classes still rely on it) but changed the behaviour. We can re-visit later confirm if this is the right approach, I'm just aiming to get the tests green first.

@andyw8andyw8 mentioned this pull request Aug 9, 2023
29 tasks
@andyw8
andyw8force-pushed the andyw8/migrate-document-to-use-yarp branch from 4f28646 to 16281e7CompareAugust 9, 2023 15:08
@andyw8
andyw8 marked this pull request as ready for review August 9, 2023 15:14
@andyw8
andyw8 requested a review from a team as a code ownerAugust 9, 2023 15:14
@github-actions

Copy link
Copy Markdown
Contributor

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

Comment threadlib/ruby_lsp/document.rb Outdated
@andyw8
andyw8force-pushed the andyw8/migrate-document-to-use-yarp branch from ccc785a to 7ccd851CompareAugust 9, 2023 16:49
@github-actions

Copy link
Copy Markdown
Contributor

@andyw8
andyw8force-pushed the andyw8/migrate-document-to-use-yarp branch from 7ccd851 to ccc785aCompareAugust 9, 2023 17:02
@github-actions

Copy link
Copy Markdown
Contributor

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

andyw8and others added 2 commits August 9, 2023 13:31
Co-authored-by: Alexandre Terrasa <583144+Morriar@users.noreply.github.com>
@andyw8
andyw8force-pushed the andyw8/migrate-document-to-use-yarp branch from 6db6970 to af350b4CompareAugust 9, 2023 17:32
@github-actions

Copy link
Copy Markdown
Contributor

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

Comment threadlib/ruby_lsp/document.rb Outdated
sig { returns(T::Boolean) }
def parsed?
!@tree.nil?
!!@parse_result.value

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get rid of this method, there's no value in keeping it anymore. We parse the document the moment it's instantiated and because YARP always succeeds in returning an AST, this method will always return true no matter what.

Comment threadtest/document_test.rb Outdated
end

def test_parsed_returns_false_when_parsing_fails
def test_parsed_returns_true_when_parsing_fails

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can get rid of this one when removing parsed?.

Comment threadtest/document_test.rb
end

def test_parsed_returns_true_when_parsed_successfully
# We are just ensuring that no error is raised

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this test. If instantiating a new document with a valid Ruby string is failing, then pretty much every other test will also fail.

@andyw8
andyw8 merged commit 0b1e46b into yarpAug 9, 2023
@andyw8
andyw8 deleted the andyw8/migrate-document-to-use-yarp branch August 9, 2023 18:06
@github-actions

Copy link
Copy Markdown
Contributor

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@andyw8@Morriar@vinistock