Simplify mockery and add check to include state in failure message #449
Conversation
since the instances method initializes it to empty if it's nil anyway
initialize @instances to an empty array on first call to setup since that should be the first site to reference instances.
nitishr
force-pushed
the
simplify-mockery
branch
from
f8b184b
to
7595826
Compare
... the check is redundant as the check method already returns early/does nothing if the action is allowed
nitishr
force-pushed
the
simplify-mockery
branch
from
7595826
to
9a1507a
Compare
|
You've created quite a few pull requests like this one which seem to be focussed on refactoring the code to make it simpler and/or remove duplication. I really appreciate all the time you've spent working on the project. However, it would help me if I understood the motivations behind pull requests like this one. For example, are you making the changes to make adding some specific new functionality easier or is it more for an abstract idea of making the code better...? Can you try to explain these motivations in the relevant pull request descriptions...? Also, if you're interested, I'd like to invite you to a Slack channel where we can more easily discuss the project. Can you send your email address to me (james "dot" mead "at" gofreerange "dot" com)? |
|
Sure, @floehopper. I have a few motivations for PRs like this:
Hope that gives you some context. Let me know if you'd like to dig into any of those further. And again, if you don't find such PRs useful, let me know and I can lower the noise. |
|
@nitishr Thanks so much for explaining - it's really helpful to understand what you're trying to achieve. And, once again, I'd like to make it clear that I very much appreciate all the work you've put in. I will do my best to review your PRs as and when I have time. |
| backtrace = if unsatisfied_expectations.empty? | ||
| caller | ||
| else | ||
| unsatisfied_expectations[0].backtrace | ||
| end | ||
| raise ExpectationErrorFactory.build(message, backtrace) | ||
| raise ExpectationErrorFactory.build("not all expectations were satisfied\n#{mocha_inspect}", backtrace) |
There was a problem hiding this comment.
I'm not convinced about the long lines this generates.
| else | ||
| unsatisfied_expectations[0].backtrace | ||
| end | ||
| backtrace = unsatisfied_expectations.empty? ? caller : unsatisfied_expectations[0].backtrace |
There was a problem hiding this comment.
Again I'm not convinced about the long lines this generates.
lib/mocha/mockery.rb
Outdated
| ['states', state_machines] | ||
| ].each do |label, list| | ||
| message << "#{label}:\n- #{list.map(&:mocha_inspect).join("\n- ")}\n" unless list.empty? | ||
| end |
There was a problem hiding this comment.
While removing the duplication here is good, I'm not convinced that the resultant code is very easy to follow.
|
As a first step, I've rebased the branch against |
I've now removed a couple of commits, re-written some of the commit messages, and force-pushed in preparation for merging. |
|
Manually merged. |
While most of the changes here are refactorings, there're 2 things in particular that go beyond and I think are useful irrespective of the other changes:
statesto be output.testcheck for includingstatesin the mocha_inspect output here: 031309fSome of the refactorings like inlining temps might be considered merely stylistic preference, although I did them based on my (admittedly limited) understanding of cognition, and I'd happily revert those if need be. Others like the ones to do with @instances are, to my mind, genuine simplifications.