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

prototype rb: Extract instance variables and class variables #1343

Merged
merged 8 commits into from
Jun 26, 2023

Conversation

tk0miya
Copy link
Contributor

@tk0miya tk0miya commented May 25, 2023

No description provided.

@tk0miya
Copy link
Contributor Author

tk0miya commented May 25, 2023

This patch generates the definition of instance variables and class variables just after the method definition:

# source.rb
class Foo
  def meth
    @_meth = 1
  end

  private

  def private_meth
    @_private_meth = 2
  end
end
# generated.rbs
class Foo
  def meth: () -> untyped

  @_meth: untyped
end

It seems fine.

But in another case, it's a bit strange for me if variables are defined inside a private method.

# source.rb
class Foo
  def meth
    @_meth = 1
  end

  private

  def private_meth
    @_private_meth = 2
  end
end
# generated.rbs
class Foo
  def meth: () -> untyped

  @_meth: untyped

  private

  def private_meth: () -> untyped

  @_private_meth: untyped
end

Is this acceptable? Or, do we need to reorder the definition before generation?
Please let me know your opinion.

Thanks,

@tk0miya
Copy link
Contributor Author

tk0miya commented May 26, 2023

Is this acceptable? Or, do we need to reorder the definition before generation?
Please let me know your opinion.

Note: After this post, I and @soutaro talked about this topic in the ruby-jp slack. And we determined to move these variable definitions to the top of the class. So I'll update this PR this weekend.

To generate natural .rbs files, this moves instance and class variable
definitions to the top of the class definition.
@tk0miya
Copy link
Contributor Author

tk0miya commented May 27, 2023

I just update my PR to sort the member of class definitions. Now instance variables and class variables will be generated at the top of each class.

@pocke
Copy link
Member

pocke commented Jun 8, 2023

Would you be able to support class instance variables?

class C
  def self.foo
    @foo = 42
  end

  class << self
    def bar
      @bar = 42
    end
  end

  @baz = 42
end

In this case, @foo, @bar, and @baz should be class instance variables, but rbs prototype rb outputs them as instance variables.

@tk0miya
Copy link
Contributor Author

tk0miya commented Jun 8, 2023

Thank you for reviewing. Good point! I tried to convert instance variables into class variables. Could you check this again?

@tk0miya
Copy link
Contributor Author

tk0miya commented Jun 8, 2023

It seems CI for the nightly image failed because installing the stringio gem failed.

lib/rbs/prototype/rb.rb Outdated Show resolved Hide resolved
lib/rbs/prototype/rb.rb Outdated Show resolved Hide resolved
Co-authored-by: Masataka Pocke Kuwabara <p.ck.t22@gmail.com>
@tk0miya
Copy link
Contributor Author

tk0miya commented Jun 15, 2023

Updated. But, after the update, I don't have a way to promote class instance variables placed just inside the class definition:

class C
 @var = nil
end

Before the update, I used the context not having a namespace...

Copy link
Member

@pocke pocke left a comment

Choose a reason for hiding this comment

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

Thanks!

@pocke
Copy link
Member

pocke commented Jun 26, 2023

Updated. But, after the update, I don't have a way to promote class instance variables placed just inside the class definition:

I understand. I'll try fixing this problem on another PR.

@pocke pocke added this pull request to the merge queue Jun 26, 2023
Merged via the queue into ruby:master with commit b32c335 Jun 26, 2023
31 checks passed
@tk0miya tk0miya deleted the extract_ivars branch June 26, 2023 06:16
@soutaro soutaro added this to the RBS 3.2 milestone Jul 18, 2023
@soutaro soutaro added the Released PRs already included in the released version label Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Released PRs already included in the released version
Development

Successfully merging this pull request may close these issues.

3 participants