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.

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

Conversation

amomchilov
Copy link
Contributor

Motivation

Closes #1918, depends on Shopify/rbi#308.

Implementation

Tests

@amomchilov amomchilov requested a review from a team as a code owner June 19, 2024 21:57
@amomchilov amomchilov requested review from Morriar and KaanOzkan June 19, 2024 21:57
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

Yay, the changes look great. Just a small test improvement suggestion.


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?

@@ -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.

@@ -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
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.

Comment on lines +297 to +301
$stderr.puts(<<~MSG)
WARNING: Attributes cannot have more than one sig.

#{e.node.string.chomp}
MSG
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

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.

Rewrite attr_reader and friends into def methods
3 participants