Skip to content
This repository

Remove unnecessary assignment from RSpec::Core::Let.get_constant_or_yield #764

Merged
merged 1 commit into from over 1 year ago

3 participants

Gabriel Sobrinho Bradley Schaefer Myron Marston
Gabriel Sobrinho

Until travis is building, it's passing on my machine:

# rspec spec/rspec/core/let_spec.rb 
Run options:
  include {:focus=>true}
  exclude {:ruby=>#<Proc:./spec/spec_helper.rb:114>}

All examples were filtered out; ignoring {:focus=>true}
........

Finished in 0.01925 seconds
8 examples, 0 failures

Randomized with seed 47898
Bradley Schaefer
Collaborator

Maybe it was meant as documentation?

Gabriel Sobrinho

@soulcutter I'm not sure if that was the intention but I'm not comfortable to document arguments like that, seems wrong for me.

Myron Marston

Maybe it was meant as documentation?

Indeed: #757 (comment)

false on its own doesn't communicate anything. If the method argument name is include_ancestors false means something completely different than if the method argument name is exclude_ancestors. I try to make my code readable on its own with no need to check API docs for the methods that are called. That's why I included the assignment: to give the argument a name.

I misread your comment...I thought you were suggesting this:

if example_group.const_defined?(name, (check_ancestors = false))
  example_group.const_get(name, check_ancestors)
else
  yield
end

...since the second assignment is unnecessary and your comment only mentioned the const_get line.

Anyhow, what about either what I thought you were suggesting or this?

check_ancestors = false
if example_group.const_defined?(name, check_ancestors)
  example_group.const_get(name, check_ancestors)
else
  yield
end

Thoughts?

Bradley Schaefer
Collaborator

I probably would have written it as the latter code snippet, @myronmarston . Any which way, does this really need 'fixing' ? It's basically all the same

Gabriel Sobrinho

It's not a big deal, it's just a detail since all do the same ;)

Take a look again @myronmarston :)

Gabriel Sobrinho

Feel free to reject if you think that's not necessary :heart:

Myron Marston myronmarston merged commit c578697 into from December 31, 2012
Myron Marston myronmarston closed this December 31, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Dec 31, 2012
Gabriel Sobrinho Remove unnecessary assignment from RSpec::Core::Let.get_constant_or_y…
…ield
ae4546c
This page is out of date. Refresh to see the latest.

Showing 1 changed file with 1 addition and 1 deletion. Show diff stats Hide diff stats

  1. 2  lib/rspec/core/let.rb
2  lib/rspec/core/let.rb
@@ -49,7 +49,7 @@ def self.get_constant_or_yield(example_group, name)
49 49
         # uses a `let` should get its own `LetDefinitions` module.
50 50
         def self.get_constant_or_yield(example_group, name)
51 51
           if example_group.const_defined?(name, (check_ancestors = false))
52  
-            example_group.const_get(name, (check_ancestors = false))
  52
+            example_group.const_get(name, check_ancestors)
53 53
           else
54 54
             yield
55 55
           end
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.