-
Notifications
You must be signed in to change notification settings - Fork 27
Preserve heredoc body when translating multi-line T.let assertions
#943
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
Open
styrmis
wants to merge
1
commit into
main
Choose a base branch
from
06-08-fix-spoom-heredoc-handling
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+84
−1
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -103,10 +103,12 @@ def maybe_translate_assertion(node) | |
| # Otherwise, replace up to the end of the node | ||
| end_offset = comment_end_offset || node.location.end_offset | ||
|
|
||
| heredoc_body = heredoc_body_within_range(value, end_offset) | ||
|
|
||
| replacement = if node.name == :bind | ||
| "#{rbs_annotation}#{trailing_comment}" | ||
| else | ||
| "#{dedent_value(node, value)} #{rbs_annotation}#{trailing_comment}" | ||
| "#{dedent_value(node, value)} #{rbs_annotation}#{trailing_comment}#{heredoc_body}" | ||
| end | ||
|
|
||
| @rewriter << Source::Replace.new(start_offset, end_offset - 1, replacement) | ||
|
|
@@ -212,6 +214,47 @@ def extract_trailing_comment(node) | |
| [" #{range.pack("C*")}", end_offset] | ||
| end | ||
|
|
||
| #: (Prism::Node, Integer) -> String? | ||
| def heredoc_body_within_range(node, replace_end_offset) | ||
| heredoc_end = find_heredoc_end_offset(node) | ||
| return unless heredoc_end | ||
| return if heredoc_end > replace_end_offset | ||
|
|
||
| value_end = node.location.end_offset | ||
| opener_line_end = value_end | ||
| opener_line_end += 1 while opener_line_end < @ruby_bytes.size && @ruby_bytes[opener_line_end] != LINE_BREAK | ||
| return if opener_line_end >= @ruby_bytes.size | ||
|
|
||
| body_bytes = @ruby_bytes[(opener_line_end + 1)...heredoc_end] #: as !nil | ||
| body = body_bytes.pack("C*") | ||
| body.chomp! if @ruby_bytes[replace_end_offset] == LINE_BREAK | ||
| "\n#{body}" | ||
| end | ||
|
|
||
| #: (Prism::Node) -> Integer? | ||
| def find_heredoc_end_offset(node) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we have something like both = T.let(
foo(<<~A, <<~B),
first
A
second
B
String,
)stopping at the first heredoc would end up dropping subsequent ones. I think we can support this case too. Something like #: (Prism::Node) -> Array[Integer]
def heredoc_end_offsets(node)
offsets = [] #: Array[Integer]
case node
when Prism::StringNode, Prism::InterpolatedStringNode
opening = node.opening_loc
closing = node.closing_loc
if opening && closing && opening.start_line != closing.start_line
offsets << closing.end_offset
end
end
node.each_child_node do |child|
offsets.concat(heredoc_end_offsets(child))
end
offsets
endUsing heredoc_end = heredoc_end_offsets(node)
.select { |offset| offset <= replace_end_offset }
.max
return unless heredoc_end |
||
| case node | ||
| when Prism::StringNode, Prism::InterpolatedStringNode | ||
| closing = node.closing_loc | ||
| opening = node.opening_loc | ||
| if closing && opening && opening.start_line != closing.start_line | ||
| return closing.end_offset | ||
| end | ||
| when Prism::CallNode | ||
| receiver = node.receiver | ||
| if receiver | ||
| result = find_heredoc_end_offset(receiver) | ||
| return result if result | ||
| end | ||
| node.arguments&.arguments&.each do |arg| | ||
| found = find_heredoc_end_offset(arg) | ||
| return found if found | ||
| end | ||
| end | ||
|
|
||
| nil | ||
| end | ||
|
|
||
| #: (Prism::Node, Prism::Node) -> String | ||
| def dedent_value(assign, value) | ||
| if value.location.start_line == assign.location.start_line | ||
|
|
||
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use
spoom/lib/spoom/sorbet/translate/translator.rb
Line 55 in a2a5712