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

respond_to? should be instance method #757

Closed
wants to merge 2 commits into from

Conversation

Yuki-Inoue
Copy link
Contributor

No description provided.

@benlangfeld
Copy link
Collaborator

Could you explain why, and perhaps propose a test to demonstrate how this is broken?

@Yuki-Inoue
Copy link
Contributor Author

Currently, it does not break anything, as far as I can tell, if the whenever is to be invoked from commandline.

However, in ruby it is always better to have consistent response between method call and the respond_to? method. Some blog posts explaining this are: http://blog.enriquez.me/2010/2/21/dont-forget-about-respond-to-when-implementing-method-missing/

Actually, I currently have one motivation to fix this; I want to query job_list whether the method identifier is available against this class.

Consider following snippet:

bundle exec ruby -I lib -r whenever -r whenever/job_list -e "job_list = Whenever::JobList.new('set :identifier, %(foo)'); puts job_list.respond_to? :identifier; puts job_list.identifier"

When executed at top level of this repo, before this fix, it prints:

false
foo

With this fix, it prints:

true
foo

Without this fix, I'd have to actually call JobList#identifier in order to determine whether identifier exist, and rescue NoMethodError (or, created a method called identifier_defined?) for those cases when :identifier is not set in the schedule.rb. Generally, it is not good to use exceptions unless they really are the exceptional cases.

With this fix, I can determine by just calling job_list.respond_to? :identifier.
(Which is actually done on here on the PR #758)

P.S. And while I was looking for blogs explaining how respond_to? work, I found we should use respond_to_missing? instead of respond_to?, as stated in https://stackoverflow.com/questions/13793060/respond-to-vs-respond-to-missing

So, I've fixed it and pushed it to this PR.

@benlangfeld
Copy link
Collaborator

Hi @Yuki-Inoue . I'd still like to see a test for this. With that and a changelog entry, I'll get this merged right away.

Base automatically changed from master to main January 20, 2021 18:17
@KjellMorgenstern
Copy link

irb actually has a problem here.

Start the irb:

Running via Spring preloader in process 141262
Loading development environment (Rails 7.0.4)
irb(main):001:0* helper.

The moment I type the dot in helper. , irb exits with a backtrace:

./home/user/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/whenever-1.0.0/lib/whenever/job_list.rb:41:in `respond_to?': undefined method `has_key?' for nil:NilClass (NoMethodError)
                           
      @set_variables.has_key?(name) || super
                    ^^^^^^^^^
        from /home/user/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/irb-1.5.0/lib/irb/completion.rb:364:in `block in retrieve_completion_data'
	from /home/user/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/irb-1.5.0/lib/irb/completion.rb:362:in `each_object'
	from /home/user/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/irb-1.5.0/lib/irb/completion.rb:362:in `retrieve_completion_data'
	from /home/user/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/irb-1.5.0/lib/irb/completion.rb:161:in `block in <module:InputCompletor>'

Copy link
Contributor

@bragamat bragamat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yuki-Inoue can you provide some update on this or @benlangfeld can we close this one out ? 😄

@Yuki-Inoue
Copy link
Contributor Author

Sorry, recently, I don't have much spare time.
I guess this PR can be closed, until someone (or future me, maybe) comes up with test.

@bragamat
Copy link
Contributor

Sorry, recently, I don't have much spare time. I guess this PR can be closed, until someone (or future me, maybe) comes up with test.

Thank you for the update @Yuki-Inoue!

@benlangfeld let's close this one as well 😄

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

4 participants