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

Use prism v0.26.0 #1953

Merged
merged 1 commit into from
Apr 22, 2024
Merged

Use prism v0.26.0 #1953

merged 1 commit into from
Apr 22, 2024

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Apr 18, 2024

Motivation

Closes #1906

@andyw8 andyw8 added the server This pull request should be included in the server gem's release notes label Apr 18, 2024
Gemfile.lock Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
module Prism
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking with Kevin about this.

@@ -124,7 +124,7 @@ def extract_document_link(node)
match = comment.location.slice.match(%r{source://.*#\d+$})
return unless match

uri = T.cast(URI(T.must(match[0])), URI::Source)
uri = T.cast(URI(match[0]), URI::Source)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorbet was complaining about the T.must on untyped, but I'm not sure why it wasn't previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(previously it was T.nilable(String).

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should cast location.slice as String instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(@st0012 is investigating the underlying cause).

Copy link
Member

Choose a reason for hiding this comment

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

This was caused by Tapioca dropping signatures of attr_reader methods (issue). I've opened ruby/prism#2719 to convert them into normal method declarations (which Tapioca does anyway).

I also tested that when generating the rbi file from that branch, this change will not be necessary.

@andyw8 andyw8 added chore Chore task dependencies dependencies and removed chore Chore task labels Apr 19, 2024
@@ -83,7 +83,9 @@ def assert_corrects_to_expected(diagnostic_code, code_action_title, source, expe
)

diagnostics = RubyLsp::Requests::Diagnostics.new(global_state, document).perform
diagnostic = T.must(T.must(diagnostics).find { |d| d.code == diagnostic_code })
# The source of the returned attributes may be RuboCop or Prism. Prism diagnostics don't have a code.
Copy link
Contributor Author

@andyw8 andyw8 Apr 19, 2024

Choose a reason for hiding this comment

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

The latest Prism returns an additionanal diagnostic:

#<LanguageServer::Protocol::Interface::Diagnostic:0x000000012d318b88
  @attributes=
   {:range=>
     #<LanguageServer::Protocol::Interface::Range:0x000000012d318d40
      @attributes=
       {:start=>#<LanguageServer::Protocol::Interface::Position:0x000000012d318f48 @attributes={:line=>0, :character=>0}>,
        :end=>#<LanguageServer::Protocol::Interface::Position:0x000000012d318d90 @attributes={:line=>0, :character=>1}>}>,
    :severity=>2,
    :source=>"Prism",
    :message=>"assigned but unused variable - a"}>,

I'm not yet sure if this impacts any other Ruby LSP behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

It should just surface these warnings as diagnostics, that should be fine.

@andyw8 andyw8 marked this pull request as ready for review April 19, 2024 15:23
@andyw8 andyw8 requested a review from a team as a code owner April 19, 2024 15:23
@andyw8 andyw8 requested review from st0012 and vinistock April 19, 2024 15:23
freebsd-git pushed a commit to freebsd/freebsd-ports that referenced this pull request Apr 20, 2024
- Bump PORTREVISION for package change

Obtained from:	Shopify/ruby-lsp#1953
@andyw8
Copy link
Contributor Author

andyw8 commented Apr 22, 2024

I think we can merge this and update again after the next Prism release?

@@ -83,7 +83,9 @@ def assert_corrects_to_expected(diagnostic_code, code_action_title, source, expe
)

diagnostics = RubyLsp::Requests::Diagnostics.new(global_state, document).perform
diagnostic = T.must(T.must(diagnostics).find { |d| d.code == diagnostic_code })
# The source of the returned attributes may be RuboCop or Prism. Prism diagnostics don't have a code.
Copy link
Member

Choose a reason for hiding this comment

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

It should just surface these warnings as diagnostics, that should be fine.

@andyw8
Copy link
Contributor Author

andyw8 commented Apr 22, 2024

Something is failing in CI for build_node on Windows but I'm confident it's unrelated to this update, so I'm going to go ahead and merge.

@andyw8 andyw8 merged commit 2c4c2d6 into main Apr 22, 2024
41 of 43 checks passed
@andyw8 andyw8 deleted the andyw8/use-prism-0.26.0 branch April 22, 2024 19:01
tcberner pushed a commit to freebsd/freebsd-ports-kde that referenced this pull request Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies dependencies server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump prism version
3 participants