Support strict keyword argument matching

[wasabigeek]

Enables strict keyword argument matching in Ruby >= 2.7, as sketched out in #544 and #446 (comment). Closes #446.

TODOs:

• Add acceptance tests for existing behaviour
• Add ruby2_keywords shim
• add configuration
• add ParametersMatchers::Hash and check for strict keyword arguments there
• documentation
• fix mocha inspect to display the correct invocation / expectation

[floehopper]

This all looks good to me so far. I've made a few minor requests - I hope they all make sense. Can you combine the two commits into one and give a bit of context/motivation in the commit note so the commit stands alone. Thanks.

[wasabigeek]

Open to a better name. Note that naming this Hash caused some constant lookup failures due to namespacing e.g. in HasEntry#parse_option, so I thought it'd be safer to name this something else (instead of checking and changing all Hashes in the codebase to explicit reference the top-level Hash).

Also, should this be # @private?

[floehopper]

Open to a better name. Note that naming this Hash caused some constant lookup failures due to namespacing e.g. in HasEntry#parse_option, so I thought it'd be safer to name this something else (instead of checking and changing all Hashes in the codebase to explicit reference the top-level Hash).

That makes sense to me and I can't immediately think of a better name. 👍

Also, should this be # @private?

Yes, I think the whole class could be marked as private for documentation purposes since I think it's only used internally. That way you could remove the method-level YARD annotations.

[wasabigeek]

I'd actually prefer that we don't couple to ruby2_keywords here, but couldn't think of a significantly better alternative yet:

• The Hash is frozen, so we can't change any instance variable on it, e.g. to assign a different attribute. An alternative might be to wrap the Hash, but I'm not sure if I'd break other assumptions in the code.
• Explicitly separating keyword arguments is tricky for now because we don't want to introduce a breaking change (e.g. hash matchers like has_value will still do "loose matching"). A fair amount of matching logic also relies on iterating and shifting an array of params (e.g. any_parameters).

Would suggest to revisit if/when we can turn on strict keyword matching by default.

[floehopper]

I'd actually prefer that we don't couple to ruby2_keywords here, but couldn't think of a significantly better alternative yet:

Sorry, my brain's a bit fried - what are the downsides of coupling to ruby2_keywords?

[wasabigeek]

Mainly this statement:

This method will probably be removed at some point, as it exists only for backwards compatibility

I'd expect that it wouldn't be removed till Ruby 2 is end-of-life, but it still gives me a bit of pause, and would be nice if we could limit it to the "edges" of the code (i.e. invocation / expectation)

[floehopper]

@wasabigeek I've tried to respond to all your questions, but I haven't had a chance to review all the changes yet. I'll try to get to that tomorrow.

[wasabigeek]

Tested it in a repo and it works, but the printed error message doesn't correctly distinguish between positional hashes / keyword args, example:

# actual code
service = Service.new(a: params[:a], b: params[:b])

# test
Service.expects(:new).with(transformed_input).returns({ a: "123", b: "US" })

# unexpected invocation: Service.new(:a => "123", :b => "US")
# unsatisfied expectations:
# - expected exactly once, invoked never: Service.new(:a => "123", :b => "US")  # error message has "stripped" the hash and made it look like a keyword arg 

Will take a closer look.

[floehopper]

@wasabigeek Thanks again for all your work. I've attempted to tidy the commits up and make a few very small tweaks in a new PR, #562. Can you have a look at this and check I haven't messed anything up or missed anything important. Also can I double-check that this should be OK to release as a minor version bump, since the new behaviour is opt-in?

[wasabigeek]

It shouldn't break existing behaviour 🤞 that said I've only tested it in a Ruby 3 repo!

Would you be able to take a look at wasabigeek#2 and see if it makes sense? The way kwargs are printed when strict matching is enabled is a bit confusing at the moment 😔

[wasabigeek]

@floehopper, just pinging in case you missed the above 🙇‍♂️

[wasabigeek]

Closing in favour of #562