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

Use namespaced loaders rather than method with global scope #1481

Merged
merged 7 commits into from Apr 9, 2020

Conversation

pyrmont
Copy link
Contributor

@pyrmont pyrmont commented Apr 7, 2020

Rouge currently uses two global methods, #load_relative and #lexer_dir, defined in lib/rouge.rb to load the various files that constitute Rouge. Rather than pollute the global namespace, it would be preferable if the loading methods were within the Rouge module.

This PR adds two top-level methods to Rouge's singleton class: #load_file and #load_lexers. It is a fork of, and builds on the work of @ashmaroli in, #1430.

@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Apr 7, 2020
@pyrmont pyrmont self-assigned this Apr 7, 2020
@ashmaroli
Copy link
Contributor

Thank you for taking on this @pyrmont
I know that it is going to be insignificant but as an FYI, it is always better (memory-allocation wise) to provide as little parameters as possible without affecting readability to File.join especially since / is the default separator on all platforms including Windows.
i.e.:
File.join(variable, 'foo', 'bar', 'lipsum') will allocate more strings than
File.join(variable, 'foo/bar/lipsum')

@pyrmont
Copy link
Contributor Author

pyrmont commented Apr 8, 2020

@ashmaroli Ta! Do you think it needs anything else?

@ashmaroli
Copy link
Contributor

Technically, # @api private has no significance but to serve as a note to the adventurous..
How about moving all the load_* calls under the Rouge singleton class. That way, the load_* methods could be truly marked as private ..

module Rouge
  class << self
    def reload!
      ...
    end

    private

    def load_file(path)
      ...
    end

    load_file 'version'
  end
end

In theory, it should work seamlessly with Rouge's existing workflow. However, I've not tested it and therefore if you feel it is unnecessary or if it won't work in practice, the pull request as of this comment looks good to be merged.

@pyrmont
Copy link
Contributor Author

pyrmont commented Apr 9, 2020

@ashmaroli We can't actually make the methods private because they're called from outside Rouge (technically you could still call them using Object#send but obviously that's not something we want to be doing). While marking the methods as @api private doesn't prevent anyone else from using them, I think it's a decent compromise.

I did make one more change, though. Given that the methods are not intended to be part of a public API, I've moved them below the #reload! and #highlight methods so the intent is a little clearer structurally.

Thanks for the feedback and the initial PR!

@pyrmont pyrmont merged commit bbdf8c8 into rouge-ruby:master Apr 9, 2020
@pyrmont pyrmont removed the needs-review The PR needs to be reviewed label Apr 9, 2020
@pyrmont pyrmont deleted the feature.namespaced-loaders branch April 9, 2020 09:53
@ashmaroli
Copy link
Contributor

because they're called from outside Rouge

Yeah that's why I suggested moving the calls inside the Rouge singleton class in my comment.
Either ways, let us run with the current implementation for some time.
Thanks for handling this @pyrmont

@pyrmont
Copy link
Contributor Author

pyrmont commented Apr 9, 2020

Ah, I see what you meant. Sorry about that—my misunderstanding. Agreed to see how this goes for now :)

mattt pushed a commit to NSHipster/rouge that referenced this pull request May 21, 2020
…#1481)

Rouge currently uses two global methods defined in `lib/rouge.rb`,
`#load_relative` and `#lexer_dir`, to load the various files in the
Rouge library. Rather than add methods to the global namespace, it
would be preferable if the loading methods were within the Rouge module.

This commits adds two methods to Rouge's singleton class, `#load_file`
and `#load_lexers`, and updates `lib/rouge.rb` to use these methods.
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