Skip to content

Adjust some return types to avoid nils #463

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

Merged
merged 1 commit into from
Jan 20, 2023

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Jan 20, 2023

Motivation

Follow up from https://github.com/Shopify/ruby-lsp/pull/462/files#r1082948918

Implementation

Automated Tests

Manual Tests

@@ -198,7 +198,7 @@ def formatting(uri)
uri: String,
position: Document::PositionShape,
character: String,
).returns(T.nilable(T::Array[Interface::TextEdit]))
).returns(T::Array[Interface::TextEdit])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to the change in lib/ruby_lsp/requests/on_type_formatting.rb

def resolve_version(uri)
version = uri.gem_version
return version unless version.nil? || version.empty?

return @gem_version unless @gem_version.nil? || @gem_version.empty?

GEM_TO_VERSION_MAP[uri.gem_name]
GEM_TO_VERSION_MAP.fetch(uri.gem_name)
Copy link
Contributor Author

@andyw8 andyw8 Jan 20, 2023

Choose a reason for hiding this comment

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

Since we had T.must(resolve_version(uri)) that would mean we always expect to find the key?

Copy link
Member

Choose a reason for hiding this comment

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

I think the mistake was actually the other way around. If the source comment is for a gem that is no longer a part of the bundle, then GEM_TO_VERSION_MAP[uri.gem_name] will return nil.

Instead of doing the T.must, we need to return early in that case.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I agree with Vini here. nil in this case is a valid output. We should do an early return on line 95 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.

Ok, I've put it back to how it was previously and created a separate issue:

#464

@andyw8 andyw8 marked this pull request as ready for review January 20, 2023 18:52
@andyw8 andyw8 requested a review from a team as a code owner January 20, 2023 18:52
@@ -21,7 +21,7 @@ def initialize
@runner = T.let(RuboCopRunner.new("-a"), RuboCopRunner)
end

sig { params(uri: String, document: Document).returns(T.nilable(String)) }
sig { params(uri: String, document: Document).returns(String) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

formatted_source returns a String.

Verified

This commit was signed with the committer’s verified signature.
andyw8 Andy Waite
@andyw8 andyw8 force-pushed the andyw8/adjust-some-return-types-to-avoid-nils branch from ffaab27 to 4b16fbe Compare January 20, 2023 21:33
@andyw8 andyw8 merged commit e196726 into main Jan 20, 2023
@andyw8 andyw8 deleted the andyw8/adjust-some-return-types-to-avoid-nils branch January 20, 2023 21:40
@shopify-shipit shopify-shipit bot temporarily deployed to production February 15, 2023 20:50 Inactive
Sign up for free to 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.

None yet

3 participants