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

[bugfix] Properly detect kwargs hashes vs optional positional args #594

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

malcolmohare
Copy link
Contributor

@malcolmohare malcolmohare commented Feb 25, 2024

Attempting to fix rspec/rspec-expectations#1451

Refactor has_kw_args_in?:

  • try to use positional parameter information to determine whether a hash is kw_args
  • try to leverage information passed by the ruby2_keywords annotation
  • fallback to inspection of keyword args to determine if they look valid or not

Added backwards compatible ruby2_keywords wrapping to the necessary methods (valid? and error_for) in the spec files like so:
ruby2_keywords(:valid?) if respond_to?(:ruby2_keywords, true)

Also needed to modify how args are passed to the MSV split_args so that the special kwargs markings are not lost.

@pirj

This comment was marked as resolved.

@malcolmohare malcolmohare force-pushed the rspec-expectation-1451 branch 2 times, most recently from 75432d1 to fd1dbb6 Compare February 27, 2024 15:56
@malcolmohare
Copy link
Contributor Author

Fully backwards compatible, just need to fix 3.2 and up. The behavior of ruby2_keywords changed between 3.1 and 3.2: https://rubyreferences.github.io/rubychanges/3.2.html#keyword-argument-separation-leftovers

Comment on lines 306 to 311
def initialize(signature, args=[])
@signature = signature
@non_kw_args, @kw_args = split_args(*args)
@non_kw_args, @kw_args = split_args(args.clone)
@min_non_kw_args = @max_non_kw_args = @non_kw_args
@arbitrary_kw_args = @unlimited_args = false
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

something had to change here. Splatting the args meant the special marking for the kwargs hash gets lost. This doesnt happen to functions with the ruby2_keywords... except it won't apply here because this function doesn't take the *args input. So either the args array is cloned (which persists the special flagging) or the input to this function needs to swap to using *args. I figured cloning was the less obtrusive change. The split_args function is private so changing its signature was not something that has compatibility concerns.

@malcolmohare malcolmohare marked this pull request as ready for review February 27, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants