Replace global methods with local lambdas

[ashmaroli]

The existing method load_relative (added to the global scope) can allow loading any ,rb file relative to the Rouge library loaded into memory. Instead, opt to use a new method (also in the global scope, but benign) that just returns a string path of a *.rb file within the Rouge library loaded into memory. Also, this has to be explicitly passed to load consequently.

Extracted from #1416

[pyrmont]

@ashmaroli Why do we care whether the method can be used in this way? The security implication are the same either way, aren't they?

[ashmaroli]

@pyrmont Personally, I feel it is not good practice to have methods injected into the global namespace. Since I authored the commit that introduced the global methods, I felt it is my responsibility to mitigate its effects.

To that effect, I have converted the global methods into lambdas that can only be used locally.

[pyrmont]

@ashmaroli OK, I see what you're saying. However, if the concern is polluting the global namespace, that's still the case whether the objects are methods or lambdas. I imagine the problem being the case where a person who includes Rouge in their code wants to create a method called load_relative and then has conflicting definitions.

To that end, I think if we are going to address the problem, it makes sense for the solution to be namespaced. Was there a reason not to make this a method on the Rouge module's singleton class? I'm imagining something like:

module Rouge
  class << self
    def load_file(path)
      load File.join(Rouge::LIB_DIR, path, ".rb")
    end

    def load_lexers
      lexer_dir = File.join(Rouge::LIB_DIR, 'rouge/lexers')
      Dir.glob(File.join(lexer_dir, '*.rb')).each do |f|
        Lexers.load_lexer(f.sub(lexer_dir, ''))
      end
    end
  end
end

I'm suggesting load_file rather than load_relative so as to maintain consistency with load_lexer but I don't have a strong attachment either way.

What do you think?

[ashmaroli]

@pyrmont The original idea was to emulate Ruby's builtin require_relative but run load under the hood. At that time, I didn't know this :load_relative method would be available in the global namespace once the Rouge library was loaded into memory.

require 'rouge'
load_relative 'rouge/version'

The above script wouldn't throw an exception.

---

But after converting the method into a lambda

require 'bundler/setup'
require 'rouge'
load_relative['rouge/version']

the above script (within the repository) on being run will throw the following since the lambda is available only within the lib/rouge.rb file:

undefined local variable or method `load_relative' for main:Object (NameError)

[pyrmont]

My point was that, conceptually, I would say that the issue with adding things to the global namespace is the risk of collisions with code that wants to use methods with the same name. Redefining the method as a lambda doesn't seem to fix that problem (I'm on my phone at the moment and haven't checked). Assuming that's correct, the appropriate solution in my opinion is namespacing (or the equivalent thereof in Ruby: encapsulation within a module).

Perhaps another reason we're coming at this form different directions is that I think it's a mistake to seek to emulate the require_relative behaviour. I realise that was the initial inspiration for the method but Rouge uses dynamic loading throughout the codebase and the better approach in my opinion is for the internal loading methods to be consistent (which was what I was attempting to do with load_file and load_lexers given the existence of load_lexer).

Finally, this is completely subjective and isn't a sound basis for a decision but I find the syntax for calling lambdas somewhere in the uncanny valley of Ruby style. Like all of a sudden we're using brackets rather tha parentheses? Easy to forget, easy to miss and just sort of ugly.

[ashmaroli]

I totally get the distaste against use of brackets. I'm not a fan of Rouge.load_file because I want this sugar method / lambda to be private. Something that cannot be used outside lib/rouge.rb

[pyrmont]

@ashmaroli But why do we care if it's private? All it does is call the Kernel#load method and all Ruby code has access to that. Moreover, if making it private is the goal, isn't the appropriate solution in Ruby to use a private method? Lambdas seem like an unnecessarily creative solution to a relatively straightforward problem.

[ashmaroli]

Okay. I'll let you do the needful. Apologies for all the noise.