This repository has been archived by the owner. It is now read-only.

Load constant Foo::Bar when Bar is seen inside Foo. #184

Merged
merged 5 commits into from Jun 27, 2016

Conversation

Projects
None yet
2 participants
@mvidner
Copy link
Contributor

mvidner commented Jun 24, 2016

If we have a definition with

    RubyLint.registry.register('Foo::Bar')

and code like

    module Foo
      Bar.baz
    end

then ruby-lint should load "Foo::Bar" first, only then try "Bar".

@@ -70,13 +77,37 @@ def run(ast)
#
def after_initialize
@loaded = Set.new
@module_nesting = []
@registry ||= RubyLint.registry

This comment has been minimized.

@YorickPeterse

YorickPeterse Jun 24, 2016

Owner

Why exactly is this using ||=?

This comment has been minimized.

@mvidner

mvidner Jun 25, 2016

Contributor

Because most callers does not pass a :registry argument to ConstantLoader.new, in which case this ensures that the attribute is set to the global registry. But in the test we pass a private registry.

It works within the scheme Iterator#initialize(options) + Iterator#after_initialize but maybe I am missing a better way to do it.

This comment has been minimized.

@YorickPeterse

YorickPeterse Jun 25, 2016

Owner

@mvidner If this is only needed for the tests then you can just stub things out using something like loader.should_receive(:registry).and_return(...)

This comment has been minimized.

@mvidner

mvidner Jun 25, 2016

Contributor

Ah, you are right!

@module_nesting.push(cp.to_s)
end

def after_module(_node)

This comment has been minimized.

@YorickPeterse

YorickPeterse Jun 24, 2016

Owner

If the argument isn't needed you can just use def after_module(*).

This comment has been minimized.

@mvidner

mvidner Jun 25, 2016

Contributor

Yes, but I preferred a way which tells the reader what the API is.

end

##
# @param [RubyLint::Node] node
#
def on_const(node)
load_constant(ConstantPath.new(node).root_node[1])
try_load_constant(ConstantPath.new(node).root_node[1])

This comment has been minimized.

@YorickPeterse

YorickPeterse Jun 24, 2016

Owner

Since this method tries to load constants while dealing with nesting I'd rather have it named something like load_nested_constant.

def try_load_constant(constant)
# ["A", "B", "C"] -> ["A::B::C", "A::B", "A"]
namespaces = module_nesting.size.downto(1).map do |n|
module_nesting.take(n).join "::"

This comment has been minimized.

@YorickPeterse

YorickPeterse Jun 24, 2016

Owner

Please use parenthesis when passing arguments (to join in this case).

# ["A", "B", "C"] -> ["A::B::C", "A::B", "A"]
namespaces = module_nesting.size.downto(1).map do |n|
module_nesting.take(n).join "::"
end

This comment has been minimized.

@YorickPeterse

YorickPeterse Jun 24, 2016

Owner

Please add an empty line after an end

module_nesting.take(n).join "::"
end
namespaces.each do |ns|
load_constant(ns + "::" + constant)

This comment has been minimized.

@YorickPeterse

YorickPeterse Jun 24, 2016

Owner

Use "#{ns}::#{constant}" instead.

@registry = RubyLint::Definition::Registry.new
# hack: get the core definitions that hardcode the global registry
@registry.instance_variable_set(:@registered,
RubyLint.registry.registered.dup)

This comment has been minimized.

@YorickPeterse

YorickPeterse Jun 24, 2016

Owner

No, I don't want tests doing things such as setting/getting instance variables directly. If the purpose is to stub out whatever registry is used you should use RSpec stubs for this.

klass.inherits(defs.constant_proxy('Object', @registry))
klass.define_method('hello_foo')
end
end

This comment has been minimized.

@YorickPeterse

YorickPeterse Jun 24, 2016

Owner

Please add an empty line after this end

mvidner added some commits Jun 24, 2016

Test loading scoped constants (Foo::Bar)
If we have a definition with

    RubyLint.registry.register('Foo::Bar')

and code like

    module Foo
      Bar.baz
    end

then ruby-lint should load "Foo::Bar" first, only then try "Bar".

@mvidner mvidner force-pushed the mvidner:load-unqualified-constants branch from 02b3300 to be71c09 Jun 25, 2016

@mvidner

This comment has been minimized.

Copy link
Contributor

mvidner commented Jun 25, 2016

Thank you, updated.

On the theme of code style, if you pick a RuboCop config, I will be happy to apply it.

mvidner added some commits Jun 24, 2016

Use a private registry so that Foo::Bar is independent across tests.
otherwise there would be a conflict between
  spec/ruby-lint/constant_loader_spec.rb
  spec/ruby-lint/virtual_machine/classes/extending_spec.rb

@mvidner mvidner force-pushed the mvidner:load-unqualified-constants branch from be71c09 to 1058d7e Jun 25, 2016

@YorickPeterse YorickPeterse merged commit d893905 into YorickPeterse:master Jun 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@YorickPeterse

This comment has been minimized.

Copy link
Owner

YorickPeterse commented Jun 27, 2016

Thanks!

@mvidner mvidner deleted the mvidner:load-unqualified-constants branch Jun 27, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.