Conversation
paracycle
left a comment
paracycle
left a comment
There was a problem hiding this comment.
Yay, the changes look great. Just a small test improvement suggestion.
|
|
||
| write!("lib/foo.rb", <<~RBI) | ||
| class Foo | ||
| attr_reader :i |
There was a problem hiding this comment.
Can we add the full set of attr_writer, attr_reader and attr_accessor to this test please?
|
|
||
| begin | ||
| tree.replace_attributes_with_methods! | ||
| rescue RBI::UnexpectedMultipleSigsError => e |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
We should discard the tree and return the original file as we do below in case of conflicts.
|
|
||
| begin | ||
| tree.replace_attributes_with_methods! | ||
| rescue RBI::UnexpectedMultipleSigsError => e |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
| $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 |
There was a problem hiding this comment.
^ Since we have the node we should also give the location
3 participants
Motivation
Closes #1918, depends on Shopify/rbi#308.
Implementation
Tests