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

monitor is require for SneakersAdapter #21878

Merged
merged 1 commit into from Oct 9, 2015

Conversation

Gaurav2728
Copy link
Contributor

we are using @monitor = Monitor.new that inherit from Monitor class, we leave behind this commit cbfc8b36

we are using `@monitor = Monitor.new` that inherit from Monitor class, we leave behind this commit rails@cbfc8b36
@rails-bot
Copy link

r? @matthewd

(@rails-bot has picked a reviewer for you, use r? to override)

@Gaurav2728
Copy link
Contributor Author

@matthewd 🙇

@rafaelfranca
Copy link
Member

Please don't ping the reviewers. I could accept that if it is a bug fix that you might be having in your application but it doesn't seem like it is. The pull request was assigned and when the reviewer can review and merge your pull request I"m sure they will.

@Gaurav2728
Copy link
Contributor Author

@rafaelfranca 👨 👌

@thedarkone
Copy link
Contributor

Good news everyone! I just checked and there is no need for a require at all (at least for ruby >= 2.0):

> ruby -e 'puts Monitor' -v
ruby 2.0.0p353 (2013-11-22 revision 43784) [x86_64-darwin12.2.0]
Monitor
> ruby -e 'puts Monitor' -v
ruby 2.2.2p95 (2015-04-13 revision 50295) [x86_64-darwin12.0]
Monitor
> ruby -e 'puts Monitor' -v
jruby 9.0.0.0-SNAPSHOT (2.2.2) 2015-05-14 da24a36 Java HotSpot(TM) 64-Bit Server VM 25.60-b23 on 1.8.0_60-b27 +jit [darwin-x86_64]
Monitor

@Gaurav2728 if you want, you can modify your PR to remove require 'monitor' from other files as well.

matthewd added a commit that referenced this pull request Oct 9, 2015
monitor is require for SneakersAdapter
@matthewd matthewd merged commit 30bacc2 into rails:master Oct 9, 2015
@matthewd
Copy link
Member

matthewd commented Oct 9, 2015

@thedarkone I think that's because rubygems happens to require it, which I guess we shouldn't rely on.

% ruby --disable-gems -e 'puts Monitor' -v
ruby 2.2.3p173 (2015-08-18 revision 51636) [x86_64-darwin14]
-e:1:in `<main>': uninitialized constant Monitor (NameError)

@Gaurav2728 Gaurav2728 deleted the require_monitor branch December 28, 2015 10:35
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

5 participants