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.

Sign up for GitHub

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

feat: implement remove newlines in brackets before toggle block to on… #3005

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

feat: implement remove newlines in brackets before toggle block to on… #3005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f17c229
feat: implement remove newlines in brackets before toggle block to on…
Hungle2911 Jan 2, 2025
c5a7096
feat: handle array and hash node in block
Hungle2911 Jan 10, 2025
2ca51ef
feat: handle array node
Hungle2911 Jan 10, 2025
1be3577
fix: add method to handle lost context
Hungle2911 Jan 12, 2025
3d7019a
fix: add comma at the end for hash and array in do..end block
Hungle2911 Jan 12, 2025
e9702de
feat: handle hash node in block
Hungle2911 Jan 12, 2025
3f46d37
fix: traverse through nested block
Hungle2911 Jan 13, 2025
01f8d5d
feat: implement remove newlines in brackets before toggle block to on…
Hungle2911 Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 63 additions & 12 deletions lib/ruby_lsp/requests/code_action_resolve.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,62 @@ def create_text_edit(range, new_text)
)
end

sig do
params(
body_content: String,
body_loc: Prism::Location,
node: Prism::Node,
indentation: T.nilable(String),
).returns(String)
end
def handle_nested_structure(body_content, body_loc, node, indentation)
location = node.location
correction_start = location.start_offset - body_loc.start_offset
correction_end = location.end_offset - body_loc.start_offset
next_indentation = indentation ? "#{indentation} " : nil

transformed_content = if node.is_a?(Prism::HashNode)
transform_hash_node(node, next_indentation)
elsif node.is_a?(Prism::ArrayNode)
transform_array_node(node, next_indentation)
elsif node.is_a?(Prism::BlockNode)
recursively_switch_nested_block_styles(node, next_indentation)
else
raise "Unsupported node type: #{node.class.name}"
end

body_content[correction_start...correction_end] = transformed_content
body_content
end

sig { params(node: Prism::HashNode, indentation: T.nilable(String)).returns(String) }
def transform_hash_node(node, indentation)
elements = node.elements.map do |elem|
if elem.is_a?(Prism::AssocNode) && !elem.operator
"#{elem.key.slice} #{switch_block_body(elem.value, indentation).strip}"
elsif elem.is_a?(Prism::AssocNode) && elem.operator
"#{elem.key.slice} => #{switch_block_body(elem.value, indentation).strip}"
elsif elem.is_a?(Prism::AssocSplatNode)
"**#{elem.value&.slice}"
end
end
if indentation
"{\n#{indentation} #{elements.join(",\n#{indentation} ")},\n#{indentation}}"
else
"{ #{elements.join(", ")} } "
end
end

sig { params(node: Prism::ArrayNode, indentation: T.nilable(String)).returns(String) }
def transform_array_node(node, indentation)
elements = node.elements.map { |elem| switch_block_body(elem, indentation).strip }
if indentation
"[\n#{indentation} #{elements.join(",\n#{indentation} ")},\n#{indentation}]"
else
"[#{elements.join(", ")}]"
end
end

sig { params(node: Prism::BlockNode, indentation: T.nilable(String)).returns(String) }
def recursively_switch_nested_block_styles(node, indentation)
parameters = node.parameters
Expand All @@ -295,7 +351,8 @@ def recursively_switch_nested_block_styles(node, indentation)
source << "{ "
source << "#{parameters.slice} " if parameters
source << switch_block_body(body, nil) if body
source << "}"
source << " }"
source = source.squeeze(" ").gsub("\n", ";")
end

source
Expand All @@ -310,24 +367,18 @@ def switch_block_body(body, indentation)
start: { line: body_loc.start_line - 1, character: body_loc.start_column },
end: { line: body_loc.end_line - 1, character: body_loc.end_column },
},
node_types: [Prism::BlockNode],
node_types: [Prism::BlockNode, Prism::HashNode, Prism::ArrayNode],
Copy link
Member

Choose a reason for hiding this comment

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

We need to locate only the top level block, then descend into the body and start correcting all statements inside.

With this approach, you could end up finding an array node mixed in a bunch of other statements and then you'd miss everything else. For example, I wouldn't expect it to work for this case:

[].each do |a, b, c|
  a = []
  { b: [a] }
end

)

body_content = body.slice.dup

# If there are nested blocks, then we change their style too and we have to mutate the string using the
# relative position in respect to the beginning of the body
if nested_block.is_a?(Prism::BlockNode)
location = nested_block.location
correction_start = location.start_offset - body_loc.start_offset
correction_end = location.end_offset - body_loc.start_offset
next_indentation = indentation ? "#{indentation} " : nil

body_content[correction_start...correction_end] =
recursively_switch_nested_block_styles(nested_block, next_indentation)
if nested_block.is_a?(Prism::HashNode) || nested_block.is_a?(Prism::ArrayNode) || nested_block.is_a?(Prism::BlockNode)
handle_nested_structure(body_content, body_loc, nested_block, indentation)
else
indentation ? body_content.gsub(";", "\n") : "#{body_content.gsub("\n", ";")} "
end

indentation ? body_content.gsub(";", "\n") : "#{body_content.gsub("\n", ";")} "
end
end
end
Expand Down
47 changes: 47 additions & 0 deletions test/expectations/code_action_resolve/nested_block_with_array_do_end.exp.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
{
"params": {
"kind": "refactor.rewrite",
"title": "Refactor: Toggle block style",
"data": {
"range": {
"start": {
"line": 0,
"character": 0
},
"end": {
"line": 8,
"character": 3
}
},
"uri": "file:///fake"
}
},
"result": {
"title": "Refactor: Toggle block style",
"edit": {
"documentChanges": [
{
"textDocument": {
"uri": "file:///fake",
"version": null
},
"edits": [
{
"range": {
"start": {
"line": 0,
"character": 12
},
"end": {
"line": 8,
"character": 3
}
},
"newText": "{ |group| users.map { |user| [group.id, user.id, Time.now] } }"
}
]
}
]
}
}
}
47 changes: 47 additions & 0 deletions test/expectations/code_action_resolve/nested_block_with_hash_do_end.exp.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
{
"params": {
"kind": "refactor.rewrite",
"title": "Refactor: Toggle block style",
"data": {
"range": {
"start": {
"line": 0,
"character": 0
},
"end": {
"line": 8,
"character": 3
}
},
"uri": "file:///fake"
}
},
"result": {
"title": "Refactor: Toggle block style",
"edit": {
"documentChanges": [
{
"textDocument": {
"uri": "file:///fake",
"version": null
},
"edits": [
{
"range": {
"start": {
"line": 0,
"character": 10
},
"end": {
"line": 8,
"character": 3
}
},
"newText": "{ |user| items.each { |item| { :user_id => user.id, item_id: item.id, **status } } }"
}
]
}
]
}
}
}
9 changes: 9 additions & 0 deletions test/fixtures/nested_block_with_array_do_end.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
groups.each do |group|
Copy link
Member

Choose a reason for hiding this comment

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

Let's add some more complicated tests. The key to getting this right will be ensuring that nested arrays and hashes are being properly handled.

users.map do |user|
[
group.id,
user.id,
Time.now
]
end
end
9 changes: 9 additions & 0 deletions test/fixtures/nested_block_with_hash_do_end.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
users.map do |user|
items.each do |item|
{
:user_id => user.id,
item_id: item.id,
**status
}
end
end
84 changes: 84 additions & 0 deletions test/requests/code_action_resolve_expectations_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,90 @@ def assert_expectations(source, expected)
assert_equal(build_code_action(json_expectations(expected)), JSON.parse(actual.to_json))
end

def assert_match_to_expected(source, expected)
actual = run_expectations(source)
assert_equal(build_code_action(expected), JSON.parse(actual.to_json))
end

def test_toggle_block_do_end_to_brackets
@__params = {
kind: "refactor.rewrite",
title: "Refactor: Toggle block style",
data: {
range: {
start: { line: 0, character: 0 },
end: { line: 2, character: 3 },
},
uri: "file:///fake",
},
}
source = <<~RUBY
[1, 2, 3].each do |number|
puts number * 2
end
RUBY
expected = {
"title" => "Refactor: Toggle block style",
"edit" => {
"documentChanges" => [{
"textDocument": {
"uri": "file:///fake",
"version": nil,
},
"edits" => [{
"range" => {
"start" => { "line" => 0, "character" => 15 },
"end" => { "line" => 2, "character" => 3 },
},
"newText" => "{ |number| puts number * 2 }",
}],
}],
},
}
assert_match_to_expected(source, expected)
end

def test_toggle_block_do_end_with_hash_to_brackets
@__params = {
kind: "refactor.rewrite",
title: "Refactor: Toggle block style",
data: {
range: {
start: { line: 0, character: 0 },
end: { line: 5, character: 3 },
},
uri: "file:///fake",
},
}
source = <<~RUBY
arr.map do |a|
{
id: a.id,
name: a.name
}
end
RUBY
expected = {
"title" => "Refactor: Toggle block style",
"edit" => {
"documentChanges" => [{
"textDocument": {
"uri": "file:///fake",
"version": nil,
},
"edits" => [{
"range" => {
"start" => { "line" => 0, "character" => 8 },
"end" => { "line" => 5, "character" => 3 },
},
"newText" => "{ |a| { id: a.id, name: a.name } }",
}],
}],
},
}
assert_match_to_expected(source, expected)
end

private

def default_args
Expand Down
Loading