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

Add flatten_scopes rewriter #108

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

Conversation

jeffcarbs
Copy link
Contributor

@jeffcarbs jeffcarbs commented Dec 1, 2021

Flattens scopes, so:

module A
  class B
    FOO = 42

    class C
      module D; end
    end
  end
end
module E; end

becomes:

module A; end
class A::B
  FOO = 42
end
class A::B::C; end
module A::B::C::D; end
module E; end

lib/rbi/rewriters/flatten_scopes.rb Show resolved Hide resolved
end
end
RBI
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add one more test showing the expected behaviour when the scopes are duplicated:

Suggested change
end
end
def test_flatten_duplicated_scopes
rbi = RBI::Parser.parse_string(<<~RBI)
module A
class B; end
end
module A
class C; end
end
module A
class D; end
end
RBI
rbi.flatten_scopes!
assert_equal(<<~RBI, rbi.string)
module A; end
module A; end
module A; end
class A::B; end
class A::C; end
class A::D; end
RBI
end

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should be smarter with this using an Index but I believe we can do this later if needed.

when Tree
visit_all(node.nodes)

parent_tree = node.parent_tree
Copy link
Collaborator

Choose a reason for hiding this comment

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

My only concern with this is that if we have nested trees, we will end up moving the node to the wrong place which might get it processed again later:

For example consider this:

rbi = Tree.new do |tree1|
  tree1 << Class.new("Foo") do |cls1|
    cls1 << Tree.new do |tree2|
      tree2 << Class.new("Bar") do |cls2|
        cls2 << Method.new("bar")
      end
    end
  end
end

Running rbi.flatten_scopes! will give us something like this:

class Foo; end

class Foo::Foo::Bar
  def bar; end
end

I wonder if we should first find the root tree and put everything in it as we go instead of trying to find the parent one?

Or maybe the method should be flatten_scopes instead of flatten_scopes! and always return a new root Tree instead of modifying the current one?

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.

None yet

2 participants