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

Remove ParametersMatcher from Invocation#call_description #543

Conversation

wasabigeek
Copy link
Contributor

@wasabigeek wasabigeek commented Aug 31, 2022

This is a somewhat odd usage of ParametersMatcher, and I think unnecessarily couples it to Invocation#call_description.

I've chosen to duplicate the mocha_inspect logic for now because it seemed possible for them to work differently e.g. Invocation should never contain matchers (right?).

Does this look fine to you @floehopper?

@floehopper
Copy link
Member

This is a somewhat odd usage of ParametersMatcher, and I think unnecessarily couples it to Invocation#call_description.

I've chosen to duplicate the mocha_inspect logic for now because it seemed possible for them to work differently e.g. Invocation should never contain matchers (right?).

Does this look fine to you @floehopper?

@wasabigeek

I feel slightly uncomfortable with the amount of duplicated code, but if this is going to make it easier to refactor the parameter matching code then I can probably live with it. It would help me to know if there's something specific that this change unlocks?

@wasabigeek
Copy link
Contributor Author

wasabigeek commented Aug 31, 2022

It mainly allows Invocation and ParametersMatcher to have different APIs - currently ParametersMatcher#initialize needs an array of args or Matchers, which Invocation#arguments can fit into. But I'm hoping that Invocation can have separate #positional_arguments and #keyword_arguments (see #544), and don't want to expose a public #arguments method just to fit into ParametersMatcher's API.

We might even be able to build the descriptions completely from #positional_arguments and #keyword_arguments, which would change the logic here and make it no longer duplicated 👍

@wasabigeek
Copy link
Contributor Author

wasabigeek commented Aug 31, 2022

I think it's also worth mentioning that duplication notwithstanding, this removes a fair amount of indirection:

  • Invocation#arguments passes an Array of objects into ParametersMatcher#initialize
  • ParametersMatcher#mocha_inspect will first convert that Array into an Array of ParameterMatchers::Equals with the original object as the underlying value (my assumption here is that Invocation#arguments would not include any matchers instances, so all elements would be converted to ParameterMatchers::Equals.)
  • Array#mocha_inspect will then call ParameterMatchers::Equals#mocha_inspect, which then calls mocha_inspect on the underlying value

This change would call mocha_inspect on the original objects immediately instead.

@floehopper floehopper mentioned this pull request Sep 3, 2022
@floehopper
Copy link
Member

@wasabigeek A couple of quick things:

  1. I've just belatedly enabled PR builds on CircleCI - I only recently realised we lost this capability when we moved from TravisCI and only even more recently realised I could enable it in the CircleCI settings - so I apologise for having made you setup CircleCI on your own fork. Please feel free to open new PRs if that would make things simpler to explain.

  2. @nitishr has just submtted Use Ruby v1.9 features #547 which (amongst other things) uses Rubocop auto-correct to move to the default Style/HashSyntax style which is ruby19, i.e. "forces use of the 1.9 syntax when hashes have all symbols for keys". I thought the pervasive nature of this change might cause you issues and so I've proposed holding off on it for now.

@floehopper
Copy link
Member

floehopper commented Sep 3, 2022

@wasabigeek Thanks for explaining your motivations so clearly. That all makes sense to me. I'll get this merged shortly.

@floehopper floehopper closed this in c26be5f Sep 3, 2022
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