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

Rewrite attributes into methods #1936

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Rewrite attributes into methods #1936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions lib/tapioca/commands/abstract_gem.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,16 @@ def merge_with_exported_rbi(gem, file)

tree = gem.exported_rbi_tree

begin
tree.replace_attributes_with_methods!
rescue RBI::UnexpectedMultipleSigsError => e
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how I feel about this.

As implemented, this will leave the tree partially rewritten (up until the time when the multi-sigged attribute is found), and log this message.

Ideally, I'd just log the message from inside RBI and discard all but the first sig, but there's no existing logger API in RBI for this. Without that, I'd have to directly $stderr.puts ... from within the RBI gem, but that feels very wrong.

Suggestions?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should discard the tree and return the original file as we do below in case of conflicts.

$stderr.puts(<<~MSG)
WARNING: Attributes cannot have more than one sig.

#{e.node.string.chomp}
MSG
Comment on lines +297 to +301
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$stderr.puts(<<~MSG)
WARNING: Attributes cannot have more than one sig.
#{e.node.string.chomp}
MSG
say_error("\n\n RBIs exported by `#{gem.name}` contain errors and can't be used:", :yellow)
say_error(" #{e.node.string.chomp}", :yellow)
return file

Copy link
Collaborator

Choose a reason for hiding this comment

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

^ Since we have the node we should also give the location

end

unless tree.conflicts.empty?
say_error("\n\n RBIs exported by `#{gem.name}` contain conflicts and can't be used:", :yellow)

Expand Down
112 changes: 112 additions & 0 deletions spec/tapioca/cli/gem_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,118 @@ def foo; end
assert_success_status(result)
end


it "must generate a gem RBI and rewrites attributes from exported gem RBIs into methods" do
foo = mock_gem("foo", "0.0.1") do

write!("lib/foo.rb", <<~RBI)
class Foo
attr_reader :i
Copy link
Member

Choose a reason for hiding this comment

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

Can we add the full set of attr_writer, attr_reader and attr_accessor to this test please?

end
RBI

write!("rbi/foo.rbi", <<~RBI)
class Foo
sig { returns(Integer) }
attr_reader :i
end
RBI
end

@project.require_mock_gem(foo)
@project.bundle_install!

result = @project.tapioca("gem foo")

assert_stdout_includes(result, "create sorbet/rbi/gems/[email protected]")
puts @project.read("sorbet/rbi/gems/[email protected]")
assert_project_file_includes("sorbet/rbi/gems/[email protected]", <<~RBI.chomp)
class Foo
sig { returns(Integer) }
def i; end
end
RBI
assert_empty_stderr(result)
assert_success_status(result)
end

it "must generate a gem RBI that merges methods and attributes" do
foo = mock_gem("foo", "0.0.1") do

write!("lib/foo.rb", <<~RBI)
class Foo
extend T::Sig

sig { returns(Integer) }
attr_reader :i

def s; end
end
RBI

write!("rbi/foo.rbi", <<~RBI)
class Foo
attr_reader :i

sig { returns(String) }
attr_reader :s
end
RBI
end

@project.require_mock_gem(foo)
@project.bundle_install!

result = @project.tapioca("gem foo")

assert_stdout_includes(result, "create sorbet/rbi/gems/[email protected]")
puts result.err
puts @project.read("sorbet/rbi/gems/[email protected]")
assert_project_file_includes("sorbet/rbi/gems/[email protected]", <<~RBI.chomp)
class Foo
sig { returns(Integer) }
def i; end

sig { returns(String) }
def s; end
end
RBI
assert_empty_stderr(result)
assert_success_status(result)
end

it "logs an error message if an attribute has multiple sigs" do
foo = mock_gem("foo", "0.0.1") do
write!("lib/foo.rb", <<~RBI)
class Foo
end
RBI

write!("rbi/foo.rbi", <<~RBI)
class Foo
sig { returns(Integer) }
sig { returns(String) }
attr_reader :i
end
RBI
end

@project.require_mock_gem(foo)
@project.bundle_install!

result = @project.tapioca("gem foo")

assert_stdout_includes(result, "create sorbet/rbi/gems/[email protected]")

assert_stderr_includes(result, <<~MSG)
WARNING: Attributes cannot have more than one sig.

sig { returns(Integer) }
sig { returns(String) }
attr_reader :i
MSG
end

it "must remove outdated RBIs" do
@project.require_mock_gem(mock_gem("foo", "0.0.1"))
@project.require_mock_gem(mock_gem("bar", "0.3.0"))
Expand Down
Loading