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

Remove unused loader argument from Plugin initializer #2095

Merged
merged 1 commit into from Dec 25, 2019

Conversation

@bdewater
Copy link
Contributor

bdewater commented Dec 24, 2019

I've been working on adding Puma instrumentation to our internal metrics gem. By reading the plugin code I got it to work in a way we want (no configuration required to enable the plugin from the gem) but I noticed I had to pass a slightly confusing loader argument when initializing the plugin.

In the current version of the gem, strangely enough the Plugin class is passed as loader, while it happens inside the PluginLoader class:

plugin = cls.new(Plugin)

this seems to have been an error made during refactoring in 33e0fa9 as the original implementation made more sense:

plugin = cls.new(self)

but even at that time, it seems the loader argument was never used, hence this PR removing it.

While technically part of the plugins API:

puma/docs/plugins.md

Lines 37 to 38 in 6bb070b

Any public methods in `Puma::Plugin` are the public API that any plugin may
use.

this does not affect the documented Puma::Plugin.create do ... end way of doing things.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.
@bdewater bdewater force-pushed the bdewater:remove-plugin-loader-ivar branch from ad66c03 to 62eae26 Dec 24, 2019
@evanphx evanphx merged commit 5ce516d into puma:master Dec 25, 2019
11 of 12 checks passed
11 of 12 checks passed
OS: ubuntu-16.04 Ruby: 2.3.x OS: ubuntu-16.04 Ruby: 2.3.x
Details
build
Details
OS: ubuntu-18.04 Ruby: 2.4.x
Details
OS: ubuntu-18.04 Ruby: 2.5.x
Details
OS: ubuntu-18.04 Ruby: 2.6.x
Details
OS: macos Ruby: 2.4.x
Details
OS: macos Ruby: 2.5.x
Details
OS: macos Ruby: 2.6.x
Details
OS: windows-latest Ruby: 2.4.x
Details
OS: windows-latest Ruby: 2.5.x
Details
OS: windows-latest Ruby: 2.6.x
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nateberkopec
Copy link
Member

nateberkopec commented Dec 26, 2019

Thx ❤️

@bdewater bdewater deleted the bdewater:remove-plugin-loader-ivar branch Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.