rspec
Delegate kwargs when defining a method with `chain`
Without this, it will raise:
ArgumentError:
wrong number of arguments (given 2, expected 1)
if you try to pass a keyword arg to a chain method on Ruby 3.
~In Ruby, you must explicitly delegate keyword arguments. (See https://www.ruby-lang.org/en/news/2019/12/12/separation-of-positional-and-keyword-arguments-in-ruby-3-0/#delegation-ruby-3)~
if you try to pass a keyword arg to a chain method on Ruby 3.
As you may see, **opts delegation isn't really the best option. See https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html for more details.
A correct way to fix this is:
May I kindly ask you to separate the fix for this case from
- @divisor % to_match == 0
+ to_match % @divisor == 0
fixes? And potentially add an example that would indicate that the previous implementation was incorrect.
As you may see,
**optsdelegation isn't really the best option. See https://eregon.me/blog/2021/02/13/correct-delegation-in-ruby-2-27-3.html for more details.A correct way to fix this is:
Good catch — thanks for the link! Pushed up a fixed commit.
Minor clarification: **opts delegation is the best option if/when you only needs to support Ruby 3+ (i.e., as soon as we drop support for Ruby 2 someday :smile:), like I can do when developing apps. But in a library like this, yeah, you are right.
May I kindly ask you to separate the fix for this case from
- @divisor % to_match == 0 + to_match % @divisor == 0fixes? And potentially add an example that would indicate that the previous implementation was incorrect.
I would be happy to, except that breaks my test. The example I added here actually does indicate that the previous implementation was incorrect. Without the fix, it fails with:
Failure/Error: expect(10).to match
expected 10 to be bigger than 5 and smaller than 10 and {:inclusive=>true} and divisible by 1
As you can see, 10 is divisible by 1.
I would rather just leave off the and_divisible_by(1) part of this line altogether:
match = matcher.and_smaller_than(10, inclusive: true).and_divisible_by(1)
since it's the test would be simpler without it, but when I tried that, it raised:
TypeError:
nil can't be coerced into Integer
# ./spec/rspec/matchers/dsl_spec.rb:381:in `%'
I considered doing further refactoring to make the and_divisible_by chain optional (as it should be), but figured that would cause a maintainer to complain about out-of-scope changes and that the bug-fix option I opted for instead would be just as trivial, and more appreciated. (Would you really rather leave a logic error in the tests that may never get noticed again or fixed in a future PR for a long time, or just fix it now? :smile:)
Rather than wasting more time strategizing how best to incorporate this trivial test bug fix, may we please just include it here?
Or, if you'd prefer, would you mind just fixing the tests to however you'd like them? (I enabled the "Allow edits" option.)
SyntaxError:
/__w/rspec-expectations/rspec-expectations/spec/rspec/matchers/dsl_spec.rb:357: syntax error, unexpected tLABEL
chain :and_smaller_than do |ceiling, inclusive: false|
I suggest extracting those specs with the matcher involving block kwargs into a separate one, as we still support Ruby < 2.
You can put it inside binding.eval(<<-CODE, __FILE__, __LINE__) as some other examples do.
