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

Simplify parameter matcher for hashes #739

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

Conversation

nitishr
Copy link
Contributor

@nitishr nitishr commented Feb 16, 2025

Improves the handling of keyword and positional hash argument matching. Simplifies the logic around matching keyword and positional hashes, removes redundant code, and ensures that the behavior aligns with Ruby's keyword argument handling.

Key Changes:

  1. Simplified to_matcher Method:

    • Removed the last parameter from the to_matcher method in instance_methods.rb. This simplifies the logic and removes unnecessary complexity in determining whether a matcher is the last in a sequence.
  2. Refactored PositionalOrKeywordHash Class:

    • Removed the last_expected_value parameter and related logic from the PositionalOrKeywordHash class. The class now focuses on matching hash entries exactly and handling keyword arguments more straightforwardly.
    • Simplified the matches? method to handle both positional and keyword hashes without needing to check if the matcher is the last one.
    • Removed redundant methods like matches_last_actual_value? and last_expected_value_is_positional_hash?.
  3. Updated Tests:

    • Removed or updated tests that were checking behavior related to the last parameter, as it is no longer relevant.
    • Simplified test cases to focus on the core functionality of matching keyword and positional hashes.
    • Updated the strict_keyword_argument_matching_test.rb to reflect the new behavior, ensuring that positional and keyword arguments are matched correctly.

the previous test asserted _should_match_ and the test deleted here
asserted _should_not_match_. The reason it did not match is because the
entry did not match (due to an extra k3: 'v3'). It had nothing to do
with keyword argument matching.
Expected keyword args will always be the last args anyway. So, if a
positional hash matches keyword args but isn't the last one, the
expectation will fail regardless as the remaining args won't have any
matcher to match with.
The subclasses didn't seem to be pulling their weight anymore. A hook
method where only one of the subclasses wants to customize the hook/do
additional checks seems like a force-fitted design choice.
@nitishr nitishr force-pushed the simplify-parameter-matcher-for-hashes branch from 3d7a06c to 6ddc23b Compare February 16, 2025 22:58
@nitishr nitishr marked this pull request as ready for review February 16, 2025 23:02
@nitishr
Copy link
Contributor Author

nitishr commented Feb 17, 2025

@floehopper I started some comprehension refactoring in order to investigating #594 and #734. In the process, I noticed a number of opportunities for simplification, and the PR is the result.

@floehopper floehopper self-assigned this Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants