Add report duplicates config option for let it be #293
Conversation
| end | ||
|
|
||
| def report_duplicates | ||
| @report_duplicates ||= :warn |
There was a problem hiding this comment.
I think, the default should be false meaning that we don't need any warnings (current behaviour).
| @@ -130,6 +155,9 @@ def let_it_be(identifier, **options, &block) | |||
| super() | |||
| end | |||
| end | |||
|
|
|||
| # Used to to track method overrides | |||
| define_method("let_it_be_#{identifier}") {} | |||
There was a problem hiding this comment.
I don't like this idea. Let's try to use the information we already have to detect if there is already a let_it_be-defined method.
For example, we can check that the instance_method_defined?(identifier) and then check the location of the definition, and if it's the current file—it's defined by let_it_be. Something like that.
There was a problem hiding this comment.
Ah, I didn't realize that we can check the source file where the method is defined
Done
lHydra
force-pushed
the
add-report-duplicates-config-option-for-let_it_be
branch
from
1dd85e9
to
4806b1f
Compare
|
@palkan some weird test fails in CI... Locally there is no such problem. Can you help me, please? |
There was a problem hiding this comment.
Left a few more comments.
CI failures are unrelated. They caused by the recent Timecop release: travisjeffery/timecop#419
| end | ||
|
|
||
| def report_duplicates | ||
| @report_duplicates ||= false |
There was a problem hiding this comment.
This memoization is useless; we can skip setting to false and use attr_reader instead
There was a problem hiding this comment.
removed excess memoization
| @@ -32,6 +34,19 @@ def register_modifier(key, on: :let, &block) | |||
| LetItBe.modifiers[key] = Modifier.new(on, block) | |||
| end | |||
|
|
|||
| def report_duplicates=(value) | |||
| value = value.to_sym | |||
There was a problem hiding this comment.
What about report_duplicates = false? (For example, to temporary disable reporting).
I think, we can remove the value validation altogether and be less defensive here
There was a problem hiding this comment.
What about report_duplicates = false? (For example, to temporary disable reporting).
We could omit this value, which would be equiv
There was a problem hiding this comment.
removed the validation
lHydra
force-pushed
the
add-report-duplicates-config-option-for-let_it_be
branch
from
4806b1f
to
dd99311
Compare
What is the purpose of this pull request?
Add
report_duplicatesconfig option to avoid surprises when overriding let_it_be in a nested contextIs there anything you'd like reviewers to focus on?
I added a stub method, to track overrides (
define_method("let_it_be_#{identifier}") {}).But somehow it looks doubtful, maybe I should use another approach?
Also I decided to use the method that comes with rspec (
RSpec.warn_with) instead ofKernel.warn.Also, I'm not sure if the default option
report_duplicates = :warnis necessary?Checklist
Closes #291