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.

Sign up for GitHub

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

Jump to bottom

Use qualified method name for calls to Kernel#load #1503

Merged
merged 4 commits into from
Apr 20, 2020
Merged

Use qualified method name for calls to Kernel#load #1503

merged 4 commits into from
Apr 20, 2020

Conversation

pyrmont
Copy link
Contributor

@pyrmont pyrmont commented Apr 17, 2020

Rouge loads various components of the library using Kernel#load. In certain situations, this can cause issues where load is redefined at the global level. To ensure maximum compatibility with other libraries, this PR replaces all calls of load with qualified calls to Kernel::load.

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

pyrmont commented Apr 17, 2020

This is primarily intended to fix the issue identified in asciidoctor/asciidoctor#3623.

@mojavelinux
Copy link
Contributor

To be consistent with calls elsewhere in the library, I think the dot-notation syntax for a static method should be used instead: Kernel.load.

@jneen
Copy link
Member

jneen commented Apr 17, 2020

I think it's fine as it is.

@jneen
Copy link
Member

jneen commented Apr 17, 2020

LGTM, @pyrmont, merge as you like :]

@pyrmont
Copy link
Contributor Author

pyrmont commented Apr 18, 2020 •
edited
Loading

I can't really see a material difference but it does seem more semantically correct to call Object::send rather than Object.send so I cleaned that up. Also took the opportunity to remove redundant logic in the visual sample app.

@ashmaroli
Copy link
Contributor

Of course you know about the following better than I do, but leaving a comment just in case you overlook them..
Rouge also calls load within various lexers. For example:

rouge/lib/rouge/lexers/gherkin.rb

Lines 20 to 24 in 2b704db

# self-modifying method that loads the keywords file
def self.keywords
load File.join(Lexers::BASE_DIR, 'gherkin/keywords.rb')
keywords
end

@pyrmont
Copy link
Contributor Author

pyrmont commented Apr 18, 2020

Thanks Ash! Will make sure to include those as well :)

@mojavelinux mojavelinux mentioned this pull request Apr 19, 2020
Closed
@mojavelinux
Copy link
Contributor

One thing we've learned from the incident that led to this request is that the top-level load method can be replaced without any warning, even when Ruby warnings are enabled. I think that provides some additional justification for why using Kernel.load is more robust.

@pyrmont pyrmont merged commit 2e9c3cd into rouge-ruby:master Apr 20, 2020
@pyrmont pyrmont deleted the bugfix.load-qualified-ns branch April 20, 2020 02:54
@pyrmont pyrmont removed the needs-review The PR needs to be reviewed label Apr 20, 2020
@pyrmont
Copy link
Contributor Author

pyrmont commented Apr 20, 2020

Thanks everyone. Got this merged in.

In terms of pushing it out in a gem, my plan is to release this as part of the other updates coming in v3.19.0. On our monthly cadence, that's scheduled for release on Tuesday 12 May.

mattt pushed a commit to NSHipster/rouge that referenced this pull request May 21, 2020
@pyrmont @mattt
Use qualified method name for calls to Kernel#load (rouge-ruby#1503)
c71f7b9 
Rouge currently loads various components of the library using `Kernel#load`. In certain conditions, this can cause issues where
`load` is redefined at the global level. To ensure maximum
compatibility with other libraries, this commit replaces all calls to
`load` with qualified calls to `Kernel::load`. It also updates one call
to `Object.send` with a call to `Object::send` for consistency.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pyrmont pyrmont

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pyrmont @mojavelinux @jneen @ashmaroli